Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughThe release updates service creation options, lifecycle command typing, legacy package-command migration, marketplace parsing, API schemas, container runtime settings, and service-management UI forms. Version metadata and changelog entries identify release 0.3.3. ChangesService updates
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant CLI
participant Liaison
participant ServiceDTO
participant ServicesCmdRepo
participant Supervisor
CLI->>Liaison: submit startCmd and service options
Liaison->>ServiceDTO: validate and construct request
ServiceDTO->>ServicesCmdRepo: provide service configuration
ServicesCmdRepo->>Supervisor: configure process-group shutdown
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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
🤖 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 `@Containerfile`:
- Line 12: Refresh the APT indexes after writing nginx.list and before
installing nginx in both Containerfile (line 12) and Containerfile.test (line
12); add the same apt-get update step to each installation chain.
In `@src/infra/envs/envs.go`:
- Line 17: Update RefreshInstallableItems to fetch and reset existing
/infinite/services checkouts to InstallableServicesItemsRepoBranch before
pulling or refreshing manifests, ensuring upgrades move from v1 to v2. Preserve
the initial-clone behavior and add an upgrade test that initializes the checkout
on v1 and verifies it switches to v2.
In `@src/infra/marketplace/marketplaceQueryRepo.go`:
- Around line 348-357: Update the command-step loop in catalogItemFactory to
return errors from both migrateLegacyManifestCmdStep and
tkValueObject.NewUnixCommand failures instead of logging and continuing;
preserve the established servicesQueryRepo.go behavior of rejecting invalid
steps so callers cannot receive a partial command list, and add a regression
test covering a malformed command step.
In `@src/infra/services/servicesCmdRepo.go`:
- Around line 291-293: Replace the unsupported group={{.ExecGroup}} directive in
the Supervisor program template with a supported wrapper or launcher that sets
the target GID before exec. Preserve the existing ExecGroup behavior, verify the
managed process runs with that Unix group in the release image, and pin or
record the Supervisor version used by the image.
In `@src/infra/services/servicesQueryRepo.go`:
- Around line 415-443: Restrict the call to migrateLegacyManifestCmdStep within
parseManifestCmdSteps to stepsType serviceCmdStepTypeInstall, leaving commands
in all other step categories unchanged. Add a regression test covering a
non-install command containing install_packages, such as uninstall_packages, and
verify it is not rewritten.
In `@src/presentation/ui/presenter/overview/index.templ`:
- Around line 778-785: Extend CustomServiceAdvancedSettings to include an input
bound to service.workingDirectory, then update ServicesLiaison.Update to parse
and forward workingDirectory alongside the other service fields. Preserve the
existing update behavior while ensuring the working-directory value is no longer
dropped.
- Around line 796-804: Update the Supervisor program configuration so ExecGroup
is appended to the user value using Supervisor’s user=USER:GROUP syntax, rather
than emitting an invalid group setting. Preserve the existing ExecGroup form
field and ensure its value flows into that user configuration. Add an
integration test that runs id -g and verifies the process uses the configured
group.
🪄 Autofix
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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 27b9e4dd-c30d-450e-a7bb-2135dafb0b38
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (25)
CHANGELOG.mdContainerfileContainerfile.testcontainer/supervisord.confgo.modsrc/domain/dto/createCustomService.gosrc/domain/dto/createInstallableService.gosrc/domain/entity/installableService.gosrc/domain/entity/installedService.gosrc/infra/envs/envs.gosrc/infra/internalDatabase/model/installedService.gosrc/infra/marketplace/marketplaceCmdRepo.gosrc/infra/marketplace/marketplaceQueryRepo.gosrc/infra/marketplace/marketplaceQueryRepo_test.gosrc/infra/services/servicesCmdRepo.gosrc/infra/services/servicesCmdRepo_test.gosrc/infra/services/servicesQueryRepo.gosrc/infra/services/servicesQueryRepo_test.gosrc/presentation/api/api.gosrc/presentation/api/docs/docs.gosrc/presentation/api/docs/swagger.jsonsrc/presentation/api/docs/swagger.yamlsrc/presentation/cli/controller/services.gosrc/presentation/liaison/services.gosrc/presentation/ui/presenter/overview/index.templ
| RUN curl -skL "https://nginx.org/keys/nginx_signing.key" | gpg --dearmor >"/usr/share/keyrings/nginx-archive-keyring.gpg" \ | ||
| && echo "deb [signed-by=/usr/share/keyrings/nginx-archive-keyring.gpg] http://nginx.org/packages/debian $(lsb_release -cs) nginx" >"/etc/apt/sources.list.d/nginx.list" \ | ||
| && install_packages nginx \ | ||
| && DEBIAN_FRONTEND=noninteractive apt-get install -y nginx \ |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Refresh APT indexes after adding the NGINX source.
apt-get update synchronizes package indexes from configured sources. Both files write nginx.list after the prior update, so the following install cannot select NGINX from that new source. (manpages.debian.org)
Containerfile#L12-L12: runapt-get updateafter writingnginx.listand before installingnginx.Containerfile.test#L12-L12: run the same refresh before installingnginx.
Proposed change for both files
RUN curl -skL "https://nginx.org/keys/nginx_signing.key" | gpg --dearmor >"/usr/share/keyrings/nginx-archive-keyring.gpg" \
&& echo "deb [signed-by=/usr/share/keyrings/nginx-archive-keyring.gpg] http://nginx.org/packages/debian $(lsb_release -cs) nginx" >"/etc/apt/sources.list.d/nginx.list" \
+ && apt-get update \
&& DEBIAN_FRONTEND=noninteractive apt-get install -y nginx \📍 Affects 2 files
Containerfile#L12-L12(this comment)Containerfile.test#L12-L12
🤖 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 `@Containerfile` at line 12, Refresh the APT indexes after writing nginx.list
and before installing nginx in both Containerfile (line 12) and
Containerfile.test (line 12); add the same apt-get update step to each
installation chain.
| InstallableServicesItemsDir string = InfiniteOsMainDir + "/services" | ||
| InstallableServicesItemsRepoUrl string = "https://github.com/goinfinite/os-services" | ||
| InstallableServicesItemsRepoBranch string = "v1" | ||
| InstallableServicesItemsRepoBranch string = "v2" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Switch existing service-repository checkouts to v2.
RefreshInstallableItems uses this branch only during the initial clone. Existing /infinite/services directories run git pull on their current branch, so upgrades from 0.3.2 keep using v1 manifests. Fetch and reset the configured branch for existing checkouts before refresh. Add an upgrade test that starts on v1.
🤖 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 `@src/infra/envs/envs.go` at line 17, Update RefreshInstallableItems to fetch
and reset existing /infinite/services checkouts to
InstallableServicesItemsRepoBranch before pulling or refreshing manifests,
ensuring upgrades move from v1 to v2. Preserve the initial-clone behavior and
add an upgrade test that initializes the checkout on v1 and verifies it switches
to v2.
| for _, rawItemCmdStep := range rawItemCmdSteps { | ||
| itemCmdStep, err := tkValueObject.NewUnixCommand(rawItemCmdStep) | ||
| rawItemCmdStepStr, err := repo.migrateLegacyManifestCmdStep( | ||
| rawItemCmdStep, | ||
| ) | ||
| if err != nil { | ||
| slog.Debug(err.Error(), slog.Any("cmdStep", rawItemCmdStep)) | ||
| continue | ||
| } | ||
|
|
||
| itemCmdStep, err := tkValueObject.NewUnixCommand(rawItemCmdStepStr) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Return command-step errors instead of silently dropping steps.
When migration or NewUnixCommand validation fails, the factory continues and returns a partial command list. catalogItemFactory then accepts the manifest. InstallItem or UninstallItem can execute with required steps missing and still complete its state transition.
Return the error from both failure branches. Match src/infra/services/servicesQueryRepo.go Lines 430-467, which rejects invalid command steps. Add a regression test for a malformed command step.
Proposed fix
if err != nil {
slog.Debug(err.Error(), slog.Any("cmdStep", rawItemCmdStep))
- continue
+ return itemCmdSteps, err
}
itemCmdStep, err := tkValueObject.NewUnixCommand(rawItemCmdStepStr)
if err != nil {
slog.Debug(err.Error(), slog.Any("cmdStep", rawItemCmdStep))
- continue
+ return itemCmdSteps, err
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| for _, rawItemCmdStep := range rawItemCmdSteps { | |
| itemCmdStep, err := tkValueObject.NewUnixCommand(rawItemCmdStep) | |
| rawItemCmdStepStr, err := repo.migrateLegacyManifestCmdStep( | |
| rawItemCmdStep, | |
| ) | |
| if err != nil { | |
| slog.Debug(err.Error(), slog.Any("cmdStep", rawItemCmdStep)) | |
| continue | |
| } | |
| itemCmdStep, err := tkValueObject.NewUnixCommand(rawItemCmdStepStr) | |
| for _, rawItemCmdStep := range rawItemCmdSteps { | |
| rawItemCmdStepStr, err := repo.migrateLegacyManifestCmdStep( | |
| rawItemCmdStep, | |
| ) | |
| if err != nil { | |
| slog.Debug(err.Error(), slog.Any("cmdStep", rawItemCmdStep)) | |
| return itemCmdSteps, err | |
| } | |
| itemCmdStep, err := tkValueObject.NewUnixCommand(rawItemCmdStepStr) | |
| if err != nil { | |
| slog.Debug(err.Error(), slog.Any("cmdStep", rawItemCmdStep)) | |
| return itemCmdSteps, err | |
| } |
🤖 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 `@src/infra/marketplace/marketplaceQueryRepo.go` around lines 348 - 357, Update
the command-step loop in catalogItemFactory to return errors from both
migrateLegacyManifestCmdStep and tkValueObject.NewUnixCommand failures instead
of logging and continuing; preserve the established servicesQueryRepo.go
behavior of rejecting invalid steps so callers cannot receive a partial command
list, and add a regression test covering a malformed command step.
| {{- if .ExecGroup}} | ||
| group={{.ExecGroup}} | ||
| {{- end}} |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- candidate files ---'
git ls-files | rg -i '(^|/)(dockerfile|compose|supervis|release|image|container|go\.mod|go\.sum|makefile|readme|.*\.ya?ml)$' | head -200
printf '%s\n' '--- relevant source locations ---'
rg -n -C 6 'ExecGroup|group=|supervis|program:' src/infra/services/servicesCmdRepo.go
printf '%s\n' '--- Supervisor and image references ---'
rg -n -i -C 3 'supervisord|supervisorctl|supervisor==|supervisor[[:space:]]*[=:]|apt.*supervisor|apk.*supervisor|pip.*supervisor|FROM .*supervisor' . --glob '!vendor/**' --glob '!node_modules/**' | head -300Repository: goinfinite/os
Length of output: 11346
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- Containerfile ---'
cat -n Containerfile
printf '%s\n' '--- Containerfile.test ---'
cat -n Containerfile.test
printf '%s\n' '--- base image and package references ---'
rg -n -i -C 4 '^(FROM|RUN)|supervisor|python|apk|apt|dnf|yum|pip|uv|poetry' Containerfile Containerfile.test container Makefile dev-build.sh
printf '%s\n' '--- template and process-manager execution ---'
sed -n '250,370p' src/infra/services/servicesCmdRepo.go
sed -n '430,570p' src/infra/services/servicesCmdRepo.go
sed -n '680,780p' src/infra/services/servicesCmdRepo.go
printf '%s\n' '--- possible wrappers/plugins or group handling ---'
rg -n -i -C 3 'exec(group|.*gid)|setgid|setuid|chown|supervisord|supervisor.*plugin|group[[:space:]]*=' . --glob '!vendor/**' --glob '!node_modules/**' | head -400Repository: goinfinite/os
Length of output: 48089
🏁 Script executed:
#!/bin/bash
set -eu
python3 - <<'PY'
from urllib.request import urlopen
urls = [
"https://supervisord.org/configuration.html",
"https://raw.githubusercontent.com/Supervisor/supervisor/main/supervisor/options.py",
]
for url in urls:
print(f"\n--- {url} ---")
try:
text = urlopen(url, timeout=20).read().decode("utf-8", "replace")
except Exception as exc:
print(f"FETCH_ERROR: {exc}")
continue
lines = text.splitlines()
for i, line in enumerate(lines):
if any(term in line.lower() for term in (
"program:x", "group", "user", "unexpected", "unknown",
"program_name", "process_config",
)):
lo, hi = max(0, i - 2), min(len(lines), i + 3)
print(f"{i+1}: " + "\n".join(lines[lo:hi]))
PY
printf '%s\n' '--- Debian package metadata ---'
curl -fsSL --max-time 20 https://packages.debian.org/trixie/supervisor 2>/dev/null |
rg -n -i -C 2 'version|supervisor' | head -80 || trueRepository: goinfinite/os
Length of output: 3411
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- official configuration reference ---'
curl -ksLf --max-time 30 https://supervisord.org/configuration.html |
sed 's/<[^>]*>/ /g' |
sed 's/ / /g; s/>/>/g; s/</</g; s/&/\&/g' |
tr -s ' ' |
rg -n -i -C 4 'program:x|^ *user|^ *group|stopasgroup|Unexpected|unknown' |
head -160
printf '%s\n' '--- Supervisor 4.2.5 options source ---'
curl -ksLf --max-time 30 \
https://raw.githubusercontent.com/Supervisor/supervisor/4.2.5/supervisor/options.py |
rg -n -C 8 'process_config|program.*options|stopasgroup|user|group|Unexpected|unknown|keys' |
head -260
printf '%s\n' '--- Supervisor 4.2.5 source tree references ---'
for file in supervisor/options.py supervisor/supervisord.py supervisor/http.py; do
printf '\nFILE %s\n' "$file"
curl -ksLf --max-time 30 \
"https://raw.githubusercontent.com/Supervisor/supervisor/4.2.5/$file" |
rg -n -i -C 3 'group=|stopasgroup|user=|program.*config|unexpected|unknown' |
head -100 || true
doneRepository: goinfinite/os
Length of output: 21660
🏁 Script executed:
#!/bin/bash
set -eu
tmpdir="$(mktemp -d)"
trap 'rm -rf "$tmpdir"' EXIT
curl -ksLf --max-time 30 \
https://raw.githubusercontent.com/Supervisor/supervisor/4.2.5/supervisor/options.py \
> "$tmpdir/options.py"
printf '%s\n' '--- program parser ---'
sed -n '850,1070p' "$tmpdir/options.py"
printf '%s\n' '--- supported parameter declarations ---'
sed -n '1810,1890p' "$tmpdir/options.py"
printf '%s\n' '--- deterministic group-support check ---'
python3 - "$tmpdir/options.py" <<'PY'
import re
import sys
text = open(sys.argv[1], encoding="utf-8").read()
program_block = text[text.index("def programs_from_parser"):text.index("class ProcessConfig")]
for name in ("user", "group", "stopasgroup", "killasgroup"):
print(f"{name}: {bool(re.search(r\"get\\(section, ['\\\"]\" + name + r\"['\\\"]\", program_block))}")
param_block = text[text.index("class ProcessConfig"):]
param_block = param_block[:param_block.index("def make_process")]
print("group in ProcessConfig parameter lists:", bool(re.search(r"['\\\"]group['\\\"]", param_block)))
print("user in ProcessConfig parameter lists:", bool(re.search(r"['\\\"]user['\\\"]", param_block)))
PYRepository: goinfinite/os
Length of output: 13219
Set the Unix group through a supported launcher.
supervisord supports user= for [program:x], but it does not apply group=. Replace this setting with a wrapper or launcher that sets the GID before exec, and test the managed process group in the release image. Pin or record the Supervisor version used by that image.
🤖 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 `@src/infra/services/servicesCmdRepo.go` around lines 291 - 293, Replace the
unsupported group={{.ExecGroup}} directive in the Supervisor program template
with a supported wrapper or launcher that sets the target GID before exec.
Preserve the existing ExecGroup behavior, verify the managed process runs with
that Unix group in the release image, and pin or record the Supervisor version
used by the image.
| func (repo *ServicesQueryRepo) migrateLegacyManifestCmdStep( | ||
| rawCmdStep any, | ||
| ) (string, error) { | ||
| rawCmdStepStr, err := tkVoUtil.InterfaceToString(rawCmdStep) | ||
| if err != nil { | ||
| return "", err | ||
| } | ||
|
|
||
| return strings.ReplaceAll( | ||
| rawCmdStepStr, | ||
| "install_packages", | ||
| "DEBIAN_FRONTEND=noninteractive apt-get install -y", | ||
| ), nil | ||
| } | ||
|
|
||
| func (repo *ServicesQueryRepo) parseManifestCmdSteps( | ||
| stepsType string, | ||
| rawCmdSteps interface{}, | ||
| stepsType serviceCmdStepType, | ||
| rawCmdSteps any, | ||
| ) (cmdSteps []tkValueObject.UnixCommand, err error) { | ||
| cmdStepsMap, assertOk := rawCmdSteps.([]interface{}) | ||
| cmdStepsMap, assertOk := rawCmdSteps.([]any) | ||
| if !assertOk { | ||
| return cmdSteps, errors.New("InvalidCmdStepsStructure") | ||
| } | ||
| stepsTypeStr := string(stepsType) | ||
|
|
||
| for _, rawCmd := range cmdStepsMap { | ||
| command, err := tkValueObject.NewUnixCommand(rawCmd) | ||
| rawCommandStr, err := repo.migrateLegacyManifestCmdStep( | ||
| rawCmd, | ||
| ) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Restrict the legacy rewrite to install steps.
parseManifestCmdSteps calls migrateLegacyManifestCmdStep for every step category. The replacement can corrupt a non-install command that contains install_packages; for example, uninstall_packages becomes unDEBIAN_FRONTEND=noninteractive apt-get install -y. Apply this migration only when stepsType is serviceCmdStepTypeInstall, and add a non-install regression test.
Proposed fix
-func (repo *ServicesQueryRepo) migrateLegacyManifestCmdStep(
- rawCmdStep any,
+func (repo *ServicesQueryRepo) migrateLegacyManifestCmdStep(
+ stepsType serviceCmdStepType,
+ rawCmdStep any,
) (string, error) {
rawCmdStepStr, err := tkVoUtil.InterfaceToString(rawCmdStep)
if err != nil {
return "", err
}
+ if stepsType != serviceCmdStepTypeInstall {
+ return rawCmdStepStr, nil
+ }
+
return strings.ReplaceAll(
rawCmdStepStr,
"install_packages",
@@
- rawCommandStr, err := repo.migrateLegacyManifestCmdStep(rawCmd)
+ rawCommandStr, err := repo.migrateLegacyManifestCmdStep(
+ stepsType, rawCmd,
+ )🤖 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 `@src/infra/services/servicesQueryRepo.go` around lines 415 - 443, Restrict the
call to migrateLegacyManifestCmdStep within parseManifestCmdSteps to stepsType
serviceCmdStepTypeInstall, leaving commands in all other step categories
unchanged. Add a regression test covering a non-install command containing
install_packages, such as uninstall_packages, and verify it is not rewritten.
| @uiForm.InputField(uiForm.InputFieldSettings{ | ||
| InputType: uiForm.InputTypeText, | ||
| InputName: "execGroup", | ||
| Label: "Exec Group", | ||
| TwoWayStatePath: "service.execGroup", | ||
| HintValue: "Unix group that will be used to execute your service commands.", | ||
| HintDisplay: uiForm.InputTypeHintDisplayTooltip, | ||
| HintDisplayTooltipBackgroundColor: "os-400", | ||
| }) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
ast-grep outline src/infra/services/servicesCmdRepo.go \
--match ProcessManagerConfRebuilder --view expanded
rg -n -C5 'Exec(User|Group)|^\s*(user|group)=|ProcessManagerConfRebuilder' \
src/infra/services/servicesCmdRepo.go \
src/infra/services/servicesCmdRepo_test.go \
src/presentation/ui/presenter/overview/index.templ
# CI integration requirement:
# Generate Supervisor configuration for a service with ExecUser and ExecGroup,
# start a command that prints `id -g`, and assert the output matches ExecGroup.Repository: goinfinite/os
Length of output: 15074
🌐 Web query:
Supervisor supervisord [program:x] group option Unix group user documentation
💡 Result:
In Supervisor, there is no separate configuration option named "group" within a [program:x] section to specify a UNIX group for process execution [1][2][3]. While the [group:x] section exists, it is used only to group multiple program definitions together for easier management (such as starting or stopping them as a unit) [1][4]. To specify a UNIX group for a process, you must use the user configuration option [5][6]. Although it is named "user", it supports specifying both the user and the group in a single string, separated by a colon [5][6]. For example, to run a program as a specific user and group, you can use the following syntax in your [program:x] section [5][7]: [program:myprogram] command=/path/to/command user=username:groupname If you only provide a username (e.g., user=username), Supervisor defaults the group to the primary group associated with that user in the system's /etc/passwd file [3]. It is also important to note that switching to a different user or group requires the supervisord process itself to be running as root [8][5]. Furthermore, Supervisor does not automatically set environment variables like USER or GROUP when performing this switch, so these must be set explicitly if needed [8][7].
Citations:
- 1: https://supervisord.org/configuration.html
- 2: https://supervisor.readthedocs.io/en/latest/configuration.html
- 3: Run a command as a specific group Supervisor/supervisor#996
- 4: https://manpages.debian.org/unstable/supervisor/supervisor.1.en.html
- 5: https://supervisord.org/configuration.html?highlight=python
- 6: https://supervisord.org/configuration.html?highlight=command
- 7: https://supervisord.org/subprocess.html
- 8: https://manpages.debian.org/trixie/supervisor/supervisord.1.en.html
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the exact renderer and update path, then locate any configuration
# fixtures or tests that establish the supported Supervisor options.
sed -n '220,315p' src/infra/services/servicesCmdRepo.go
sed -n '800,890p' src/infra/services/servicesCmdRepo.go
rg -n -C3 'supervisord|supervisor|group=|exec_group|ExecGroup' --glob '!vendor/**' .Repository: goinfinite/os
Length of output: 30029
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the update DTO and all assignments to determine whether the same
# execution-group value is persisted when the service settings are edited.
rg -n -C8 'type Update|ExecGroup|execGroup|updateDto' \
src/domain/dto src/presentation src/infra/services \
--glob '*.go' --glob '*.templ'
# Show the complete configuration-template region and nearby tests.
sed -n '250,350p' src/infra/services/servicesCmdRepo.go
sed -n '1,230p' src/infra/services/servicesCmdRepo_test.go
# Deterministically model the generated Supervisor line for the supported
# user:group syntax and the current group= syntax.
python3 - <<'PY'
from dataclasses import dataclass
`@dataclass`
class Service:
user: str | None
group: str | None
def current(s: Service) -> str:
out = [f"user={s.user or 'root'}"]
if s.group:
out.append(f"group={s.group}")
return "\n".join(out)
def supervisor_user_group(s: Service) -> str:
return f"user={s.user or 'root'}{':' + s.group if s.group else ''}"
s = Service("alice", "developers")
print("current:\n" + current(s))
print("supported:\n" + supervisor_user_group(s))
PYRepository: goinfinite/os
Length of output: 50369
🏁 Script executed:
#!/bin/bash
set -euo pipefail
fd -i 'update.*service|service.*update' src/domain/dto src/presentation
rg -n -C6 'type UpdateService|NewUpdateService|execGroup|ExecGroup' \
src/domain/dto/updateService.go \
src/presentation/liaison/services.go \
src/presentation/ui/presenter/overview/index.templ
# Check whether the UI state path is handled by a generic update mechanism or
# whether the service update path lacks an ExecGroup field.
python3 - <<'PY'
from pathlib import Path
paths = [
Path("src/domain/dto/updateService.go"),
Path("src/presentation/liaison/services.go"),
Path("src/presentation/ui/presenter/overview/index.templ"),
]
for path in paths:
text = path.read_text()
print(f"{path}: ExecGroup={text.count('ExecGroup')}, execGroup={text.count('execGroup')}")
PYRepository: goinfinite/os
Length of output: 7496
Use Supervisor’s user=USER:GROUP syntax for ExecGroup.
group={{.ExecGroup}} is not a valid [program] setting. Append ExecGroup to the user value, and add an integration test that runs id -g and checks the configured group.
🤖 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 `@src/presentation/ui/presenter/overview/index.templ` around lines 796 - 804,
Update the Supervisor program configuration so ExecGroup is appended to the user
value using Supervisor’s user=USER:GROUP syntax, rather than emitting an invalid
group setting. Preserve the existing ExecGroup form field and ensure its value
flows into that user configuration. Add an integration test that runs id -g and
verifies the process uses the configured group.
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/presentation/liaison/services.go (2)
248-259: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReject non-string
startCmdvalues before conversion.
tkValueObject.NewUnixCommandconverts scalar values to strings. Therefore,truebecomesUnixCommand("true").CreateInstallableuses any non-nilstartCmdto replace the manifest command.The API schema defines
startCmdas a string. Validate the raw value type before constructing the value object. Otherwise, a JSON boolean can replace the manifest command instead of returning a user error.🤖 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 `@src/presentation/liaison/services.go` around lines 248 - 259, Validate that untrustedInput["startCmd"] is a string before passing it to tkValueObject.NewUnixCommand; return the existing user-error response for any non-string value. Only construct and assign startCmdPtr after this type check, preserving the current behavior for omitted or valid string values.
619-632: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftValidate
workingDirectorybefore persisting it.
NewUnixAbsoluteFilePath(..., false)validates only the value format. It accepts nonexistent paths and converts relative input to root-relative paths.CreateCustompersists the service before directory setup, configuration rebuild, andStart.CreateInstallablealso does not create or validate a custom working directory. Supervisor emits this value asdirectory=..., so an unusable path can leave a persisted service that cannot start.Updatepersistsworking_directorybefore rebuilding the configuration and has no rollback. Require a usable directory before persistence, or create it and set its permissions. Roll back failed create and update operations.🤖 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 `@src/presentation/liaison/services.go` around lines 619 - 632, Validate workingDirectory as a usable directory, not merely with NewUnixAbsoluteFilePath(..., false), before persisting it; create it and apply required permissions when appropriate. Update CreateCustom and CreateInstallable so directory setup and validation complete before service persistence. Update Update to perform the same checks before saving working_directory, and roll back persisted create or update state whenever setup, configuration rebuilding, or Start fails.
🧹 Nitpick comments (1)
src/presentation/liaison/services.go (1)
454-466: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAdd a scheduled
create-installableround-trip test.The CLI consumes
--start-command,--startup-file, and--working-dir. Test values with spaces and embedded single and double quotes. Assert that the downstream request receives each original value 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 `@src/presentation/liaison/services.go` around lines 454 - 466, Add a scheduled create-installable round-trip test covering --start-command, --startup-file, and --working-dir with values containing spaces and embedded single and double quotes. Execute the CLI path and assert the downstream request receives each original, unmodified value after shell escaping and parsing.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/presentation/liaison/services.go`:
- Around line 248-259: Validate that untrustedInput["startCmd"] is a string
before passing it to tkValueObject.NewUnixCommand; return the existing
user-error response for any non-string value. Only construct and assign
startCmdPtr after this type check, preserving the current behavior for omitted
or valid string values.
- Around line 619-632: Validate workingDirectory as a usable directory, not
merely with NewUnixAbsoluteFilePath(..., false), before persisting it; create it
and apply required permissions when appropriate. Update CreateCustom and
CreateInstallable so directory setup and validation complete before service
persistence. Update Update to perform the same checks before saving
working_directory, and roll back persisted create or update state whenever
setup, configuration rebuilding, or Start fails.
---
Nitpick comments:
In `@src/presentation/liaison/services.go`:
- Around line 454-466: Add a scheduled create-installable round-trip test
covering --start-command, --startup-file, and --working-dir with values
containing spaces and embedded single and double quotes. Execute the CLI path
and assert the downstream request receives each original, unmodified value after
shell escaping and parsing.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 28b58614-b15a-48a9-878a-d569f1adc3da
📒 Files selected for processing (13)
CHANGELOG.mdsrc/domain/dto/createCustomService.gosrc/domain/entity/installableService.gosrc/domain/entity/installedService.gosrc/infra/services/servicesCmdRepo.gosrc/infra/services/servicesQueryRepo.gosrc/infra/services/servicesQueryRepo_test.gosrc/presentation/api/docs/docs.gosrc/presentation/api/docs/swagger.jsonsrc/presentation/api/docs/swagger.yamlsrc/presentation/cli/controller/services.gosrc/presentation/liaison/services.gosrc/presentation/ui/presenter/overview/index.templ
💤 Files with no reviewable changes (7)
- CHANGELOG.md
- src/domain/entity/installableService.go
- src/domain/entity/installedService.go
- src/infra/services/servicesQueryRepo_test.go
- src/presentation/api/docs/docs.go
- src/domain/dto/createCustomService.go
- src/presentation/api/docs/swagger.yaml
🚧 Files skipped from review as they are similar to previous changes (3)
- src/presentation/cli/controller/services.go
- src/presentation/ui/presenter/overview/index.templ
- src/infra/services/servicesCmdRepo.go
|
1 participant
Summary
Validation
templ generate -path src/presentation/uigo test ./... -run '^$'go vet ./src/presentation/ui/presenter/overviewgit diff --checkSummary by CodeRabbit
New Features
Bug Fixes
Documentation