Conversation
# Conflicts: # go.mod # go.sum
📝 WalkthroughWalkthroughThe daemon is migrated to gRPC-only communication, introduces synchronized server-task scheduling, replaces shell-parsed commands with argument vectors, adds prioritized repository mirrors, simplifies REST-backed services, and updates configuration, enrollment, platform launchers, and tests. ChangesDaemon runtime migration
Estimated code review effort: 5 (Critical) | ~120 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
internal/app/gdaemon_scheduler/task_manager.go (2)
205-221: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winMissing predecessor now retries forever instead of eventually failing.
Previously a predecessor not found in the queue or completion tracker was resolved via the repository, which could return
predecessorFail. Now it unconditionally returnspredecessorWaitand just logs a warning. Sincecompletedis a fixed-capacity tracker (newCompletionTracker(completionTrackerCapacity)), a predecessor whose record is evicted — or a task referencing a stale/invalidRunAfterID— will wait indefinitely: the dependent task can never proceed or fail, staying in the queue and being reprocessed on every scheduler tick.Consider adding a bounded retry count or max-wait duration in this branch so the task eventually fails with a clear "predecessor not found" reason instead of looping forever.
🔧 Sketch of a bounded-wait fallback
- logger.Logger(ctx).Warnf( - "predecessor task %d not found in queue or completion tracker, will retry", - runAfterID, - ) - - return predecessorWait, "" + if task.PredecessorWaitExceeded() { // or an equivalent attempt/duration guard + logger.Logger(ctx).Errorf( + "predecessor task %d not found in queue or completion tracker, giving up", + runAfterID, + ) + return predecessorFail, "predecessor task not found" + } + + logger.Logger(ctx).Warnf( + "predecessor task %d not found in queue or completion tracker, will retry", + runAfterID, + ) + + return predecessorWait, ""🤖 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 `@internal/app/gdaemon_scheduler/task_manager.go` around lines 205 - 221, Update the missing-predecessor branch in the task manager’s predecessor evaluation flow to track retries or elapsed wait time and eventually return predecessorFail with a clear “predecessor not found” reason. Preserve the current predecessorWait behavior while the configured retry or wait bound has not been exceeded, and ensure the bound applies to stale or evicted RunAfterID records.
347-375: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winInconsistent output/fail ordering vs.
executeCommand/executeGameCommand.In
executeCommand(Lines 296-297) andexecuteGameCommand(Lines 339-340),notifyTaskOutputis called beforefailTask. Here,failTask(Line 368) runs beforenotifyTaskOutput(Line 372) for the same "final, not successful" case. IffailTasksignals task completion downstream, the last output chunk may be delivered after consumers have already stopped listening.🔧 Proposed reorder for consistency
if isFinal { manager.commandsInProgress.Delete(task.ID()) + manager.notifyTaskOutput(task, output, isFinal) if cmd.Result() == gameservercommands.SuccessResult { err := task.SetStatus(domain.GDTaskStatusSuccess) if err != nil { return err } manager.notifyTaskStatus(task, "Task completed successfully") } else { manager.failTask(ctx, task) } + + return nil } manager.notifyTaskOutput(task, output, isFinal) return nil }🤖 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 `@internal/app/gdaemon_scheduler/task_manager.go` around lines 347 - 375, In proceedTask, ensure the final output is delivered before failure completion is signaled: move notifyTaskOutput ahead of the non-success failTask call while preserving successful status handling and final-task cleanup. Keep the existing output notification arguments and ensure both successful and failed final executions retain consistent ordering with executeCommand and executeGameCommand.
🧹 Nitpick comments (1)
internal/processmanager/simple.go (1)
65-84: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winGetOutput duplicates execCommand's build+exec pattern.
GetOutputre-implements the same build-args → exec → wrap-error sequence thatexecCommand(lines 92-111) already centralizes, differing only in usingpm.executorinstead ofpm.detailedExecutor. Consider parameterizingexecCommandwith the executor to use, or adding a variant, to avoid the two copies diverging over time.♻️ Proposed consolidation
-func (pm *Simple) GetOutput( - ctx context.Context, server *domain.Server, out io.Writer, -) (domain.Result, error) { - args, err := domain.BuildCommandArgs(pm.cfg, server, pm.cfg.Scripts.GetConsole, "") - if err != nil { - return domain.ErrorResult, errors.WithMessage(err, "failed to build command") - } - - result, err := pm.executor.ExecWithWriterArgs( - ctx, - args, - out, - pm.executeOptions(server), - ) - if err != nil { - return domain.ErrorResult, errors.WithMessage(err, "failed to exec command") - } - - return domain.Result(result), nil -} +func (pm *Simple) GetOutput( + ctx context.Context, server *domain.Server, out io.Writer, +) (domain.Result, error) { + return pm.execCommandWith(ctx, pm.executor, server, pm.cfg.Scripts.GetConsole, "", out) +}Then generalize
execCommandto accept the executor, e.g.execCommandWith(ctx, executor, server, wrapper, serverCommand, out), and haveexecCommandcall it withpm.detailedExecutor.🤖 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 `@internal/processmanager/simple.go` around lines 65 - 84, Consolidate GetOutput’s duplicated command-building and execution flow with execCommand by introducing a shared executor-parameterized helper, such as execCommandWith. Preserve the existing error wrapping and command arguments, have execCommand delegate using pm.detailedExecutor, and have GetOutput use pm.executor through the shared path.
🤖 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 `@config/gameap-daemon.yaml`:
- Around line 14-16: Replace the concrete-looking value assigned to api_key in
the sample configuration with an unmistakably fake placeholder such as
your-api-key-here, while preserving the key name and surrounding configuration.
In `@internal/app/domain/server_task.go`:
- Around line 219-241: Protect all ServerTask field reads with s.mutex to match
UpdateFromOptions: update ServerID(), NodeID(), Command(), Server(),
OverlapPolicy(), CatchupPolicy(), Name(), Timezone(), Payload(), Enabled(),
UpdatedAt(), and IsActive() to lock consistently, preferably with defer unlock.
Preserve each getter’s existing return behavior while ensuring no listed field
is read lock-free.
In `@internal/app/grpc/file_handler.go`:
- Around line 275-277: Update the recursive listing logic around the os.Stat
check to reject roots that are not directories by validating info.IsDir(), and
propagate WalkDir errors when the visited path equals dirPath instead of
swallowing root-level failures. Keep valid directory listings unchanged, and add
tests covering a regular-file root and an error at path == dirPath.
In `@internal/processmanager/shawl_windows.go`:
- Around line 403-409: Stop writing the full binPath, which contains the wrapped
game command and potential credentials, to task output. Update the
output/logging in the service-creation flow around binPath and the scArgs
construction to report only safe metadata or a redacted value, while preserving
the existing vector-based sc.exe argument construction unchanged.
In `@README.md`:
- Around line 81-83: Update Config.IsInsecure() to treat cfg.GRPC.Insecure as an
insecure connection condition, ensuring certificate settings are not required
when grpc.insecure is true. Preserve the existing grpc.address and api_host
checks, and keep the README exception aligned with this validation behavior.
---
Outside diff comments:
In `@internal/app/gdaemon_scheduler/task_manager.go`:
- Around line 205-221: Update the missing-predecessor branch in the task
manager’s predecessor evaluation flow to track retries or elapsed wait time and
eventually return predecessorFail with a clear “predecessor not found” reason.
Preserve the current predecessorWait behavior while the configured retry or wait
bound has not been exceeded, and ensure the bound applies to stale or evicted
RunAfterID records.
- Around line 347-375: In proceedTask, ensure the final output is delivered
before failure completion is signaled: move notifyTaskOutput ahead of the
non-success failTask call while preserving successful status handling and
final-task cleanup. Keep the existing output notification arguments and ensure
both successful and failed final executions retain consistent ordering with
executeCommand and executeGameCommand.
---
Nitpick comments:
In `@internal/processmanager/simple.go`:
- Around line 65-84: Consolidate GetOutput’s duplicated command-building and
execution flow with execCommand by introducing a shared executor-parameterized
helper, such as execCommandWith. Preserve the existing error wrapping and
command arguments, have execCommand delegate using pm.detailedExecutor, and have
GetOutput use pm.executor through the shared path.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: d6f23116-f1ad-4615-81e7-88a1f9678e16
⛔ Files ignored due to path filters (2)
config/certs_panel/dh2048.pemis excluded by!**/*.pemgo.sumis excluded by!**/*.sum
📒 Files selected for processing (144)
.github/workflows/release.ymlREADME.mdconfig/certs/client.crtconfig/certs/client.csrconfig/certs/client.keyconfig/certs_panel/ca.crtconfig/certs_panel/server.crtconfig/certs_panel/server.csrconfig/certs_panel/server.keyconfig/gameap-daemon.yamlgo.modinternal/app/components/executor.gointernal/app/components/extendable_executor.gointernal/app/config/config.gointernal/app/config/config_test.gointernal/app/config/errors.gointernal/app/config/loader.gointernal/app/config/loader_test.gointernal/app/config/loader_unix_test.gointernal/app/config/loader_windows_test.gointernal/app/config/nodeConfig.gointernal/app/config/repository_replacements.gointernal/app/config/repository_replacements_test.gointernal/app/config/scripts.gointernal/app/config/writer.gointernal/app/config/writer_test.gointernal/app/contracts/contracts.gointernal/app/di/container.gointernal/app/di/internal/_config.gointernal/app/di/internal/container.gointernal/app/di/internal/definitions/container.gointernal/app/di/internal/definitions/contracts.gointernal/app/di/internal/definitions/grpc.gointernal/app/di/internal/definitions/repositories.gointernal/app/di/internal/definitions/services.gointernal/app/domain/api_request.gointernal/app/domain/commands.gointernal/app/domain/commands_args_test.gointernal/app/domain/errors.gointernal/app/domain/gdaemon_task.gointernal/app/domain/server_task.gointernal/app/enroll_cmd.gointernal/app/game_server_commands/install_server.gointernal/app/game_server_commands/install_server_test.gointernal/app/game_server_commands/repository_replacer.gointernal/app/game_server_commands/repository_replacer_test.gointernal/app/gdaemon_scheduler/task_manager.gointernal/app/grpc/client.gointernal/app/grpc/file_handler.gointernal/app/grpc/file_handler_test.gointernal/app/grpc/transfer_handler.gointernal/app/repositories/errors.gointernal/app/repositories/gdtask_repository.gointernal/app/repositories/server_repository.gointernal/app/repositories/server_task_repository.gointernal/app/run.gointernal/app/server/commands/commands.gointernal/app/server/commands/requests.gointernal/app/server/commands/response.gointernal/app/server/enum.gointernal/app/server/files/enum.gointernal/app/server/files/files.gointernal/app/server/files/os_utils.gointernal/app/server/files/os_utils_darwin.gointernal/app/server/files/os_utils_linux.gointernal/app/server/files/os_utils_windows.gointernal/app/server/files/requests.gointernal/app/server/files/response.gointernal/app/server/messages.gointernal/app/server/response/status.gointernal/app/server/server.gointernal/app/server/server_common/common.gointernal/app/server/status/enum.gointernal/app/server/status/response.gointernal/app/server/status/status.gointernal/app/servers_scheduler/cache.gointernal/app/servers_scheduler/execution.gointernal/app/servers_scheduler/helpers_test.gointernal/app/servers_scheduler/output.gointernal/app/servers_scheduler/output_test.gointernal/app/servers_scheduler/policy.gointernal/app/servers_scheduler/policy_test.gointernal/app/servers_scheduler/queue.gointernal/app/servers_scheduler/queue_test.gointernal/app/servers_scheduler/resync.gointernal/app/servers_scheduler/scheduler.gointernal/app/servers_scheduler/scheduler_cancel_test.gointernal/app/servers_scheduler/scheduler_catchup_test.gointernal/app/servers_scheduler/scheduler_output_test.gointernal/app/servers_scheduler/scheduler_overlap_test.gointernal/app/servers_scheduler/scheduler_test.gointernal/app/servers_scheduler/scheduler_version_test.gointernal/app/servers_scheduler/types.gointernal/app/services/api.gointernal/app/services/runner.gointernal/processmanager/docker.gointernal/processmanager/podman.gointernal/processmanager/shawl_windows.gointernal/processmanager/simple.gointernal/processmanager/systemd.gointernal/processmanager/systemd_internal_test.gointernal/processmanager/systemd_metrics_test.gointernal/processmanager/systemd_quoting_test.gointernal/processmanager/tmux.gointernal/processmanager/winsw_windows.gopkg/limiter/limiter.gopkg/limiter/limiter_test.gopkg/shellquote/unquote.gopkg/shellquote/windows_arg_test.gotest/functional/gdtasks/commands/cmdexec_test.gotest/functional/gdtasks/commands/suite_test.gotest/functional/gdtasks/suite.gotest/functional/repositoriestest/fixtures.gotest/functional/repositoriestest/gdtaskrepository/fixtures_test.gotest/functional/repositoriestest/gdtaskrepository/gdtask_repository_test.gotest/functional/repositoriestest/gdtaskrepository/suite_test.gotest/functional/repositoriestest/server_task_repository/server_task_repository_test.gotest/functional/repositoriestest/server_task_repository/suite_test.gotest/functional/repositoriestest/serverrepository/server_repository_test.gotest/functional/repositoriestest/serverrepository/suite_test.gotest/functional/repositoriestest/suite.gotest/functional/server_tasks/scheduler_grpc_test.gotest/functional/server_tasks/server_tasks_test.gotest/functional/server_tasks/suite_test.gotest/functional/servertest/commands/commands_unix_test.gotest/functional/servertest/commands/commands_windows_test.gotest/functional/servertest/commands/exec_test.gotest/functional/servertest/commands/suite_test.gotest/functional/servertest/files/chmod_test.gotest/functional/servertest/files/file_info_test.gotest/functional/servertest/files/list_test.gotest/functional/servertest/files/mkdir_test.gotest/functional/servertest/files/move_copy_test.gotest/functional/servertest/files/remove_test.gotest/functional/servertest/files/suite_test.gotest/functional/servertest/files/upload_download_test.gotest/functional/servertest/files/validation_test.gotest/functional/servertest/status/status_test.gotest/functional/servertest/status/suite_test.gotest/functional/servertest/suite.gotest/manual/client/client.gotest/mocks/gdtask_repository.gotest/mocks/server_task_repository.gotest/mocks/task_stats_reader.go
💤 Files with no reviewable changes (79)
- config/certs_panel/server.key
- config/certs_panel/ca.crt
- internal/app/server/files/os_utils_windows.go
- config/certs_panel/server.csr
- internal/app/server/enum.go
- test/functional/servertest/status/status_test.go
- internal/app/server/files/enum.go
- internal/app/config/loader.go
- internal/app/repositories/errors.go
- internal/app/server/files/os_utils_darwin.go
- test/functional/repositoriestest/gdtaskrepository/fixtures_test.go
- config/certs/client.csr
- internal/app/server/files/os_utils.go
- test/functional/servertest/commands/commands_windows_test.go
- config/certs/client.crt
- internal/app/config/loader_unix_test.go
- internal/app/server/commands/requests.go
- test/functional/servertest/status/suite_test.go
- pkg/limiter/limiter_test.go
- internal/app/di/internal/definitions/container.go
- config/certs_panel/server.crt
- test/functional/servertest/commands/commands_unix_test.go
- test/functional/servertest/files/chmod_test.go
- test/functional/repositoriestest/fixtures.go
- test/functional/servertest/files/validation_test.go
- test/mocks/gdtask_repository.go
- test/functional/servertest/commands/suite_test.go
- internal/app/server/server_common/common.go
- internal/app/server/files/os_utils_linux.go
- internal/app/repositories/gdtask_repository.go
- internal/app/domain/gdaemon_task.go
- internal/app/servers_scheduler/queue_test.go
- test/mocks/server_task_repository.go
- internal/app/server/messages.go
- internal/app/server/commands/response.go
- internal/app/server/files/files.go
- internal/app/domain/errors.go
- internal/app/config/loader_windows_test.go
- test/functional/gdtasks/commands/suite_test.go
- pkg/limiter/limiter.go
- test/mocks/task_stats_reader.go
- test/functional/servertest/files/file_info_test.go
- test/functional/servertest/files/list_test.go
- test/manual/client/client.go
- test/functional/servertest/files/mkdir_test.go
- config/certs/client.key
- internal/app/server/commands/commands.go
- test/functional/servertest/files/suite_test.go
- test/functional/servertest/files/move_copy_test.go
- test/functional/repositoriestest/gdtaskrepository/suite_test.go
- internal/app/server/status/enum.go
- test/functional/repositoriestest/serverrepository/suite_test.go
- test/functional/repositoriestest/suite.go
- internal/app/server/status/response.go
- test/functional/repositoriestest/server_task_repository/suite_test.go
- internal/app/config/nodeConfig.go
- test/functional/servertest/files/remove_test.go
- internal/app/repositories/server_task_repository.go
- internal/app/services/api.go
- internal/app/server/status/status.go
- internal/app/domain/api_request.go
- test/functional/server_tasks/server_tasks_test.go
- internal/app/config/loader_test.go
- internal/app/server/server.go
- internal/app/servers_scheduler/queue.go
- test/functional/server_tasks/suite_test.go
- test/functional/repositoriestest/gdtaskrepository/gdtask_repository_test.go
- internal/app/config/writer.go
- test/functional/servertest/files/upload_download_test.go
- test/functional/repositoriestest/serverrepository/server_repository_test.go
- test/functional/repositoriestest/server_task_repository/server_task_repository_test.go
- test/functional/servertest/commands/exec_test.go
- internal/app/server/files/requests.go
- internal/app/di/container.go
- internal/app/server/response/status.go
- internal/app/server/files/response.go
- internal/app/enroll_cmd.go
- test/functional/servertest/suite.go
- internal/app/di/internal/definitions/services.go
Comment on lines
+219
to
+241
| func (s *ServerTask) UpdateFromOptions(opts ServerTaskOptions) { | ||
| s.mutex.Lock() | ||
| defer s.mutex.Unlock() | ||
|
|
||
| s.serverID = opts.ServerID | ||
| s.nodeID = opts.NodeID | ||
| s.version = opts.Version | ||
| s.command = opts.Command | ||
| if opts.Server != nil { | ||
| s.server = opts.Server | ||
| } | ||
| s.executeDate = opts.ExecuteDate | ||
| s.repeat = opts.Repeat | ||
| s.repeatPeriod = opts.RepeatPeriod | ||
| s.counter = opts.Counter | ||
| s.overlapPolicy = opts.OverlapPolicy | ||
| s.catchupPolicy = opts.CatchupPolicy | ||
| s.name = opts.Name | ||
| s.timezone = opts.Timezone | ||
| s.payload = opts.Payload | ||
| s.enabled = opts.Enabled | ||
| s.updatedAt = opts.UpdatedAt | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Data race: UpdateFromOptions mutates fields under the mutex, but sibling getters read them lock-free.
UpdateFromOptions writes serverID, nodeID, command, server, repeat, repeatPeriod, overlapPolicy, catchupPolicy, name, timezone, payload, enabled, and updatedAt while holding s.mutex. However ServerID(), NodeID(), Command(), Server(), OverlapPolicy(), CatchupPolicy(), Name(), Timezone(), Payload(), Enabled(), UpdatedAt() (Lines 106-112, 158-184) and IsActive() (Line 212, reading s.enabled) read the same fields without the lock.
The scheduler holds shared *ServerTask pointers in the cache: tick() (ticker goroutine) reads via these getters while ApplySnapshot/ApplyDelta (gRPC handler goroutine) call UpdateFromOptions on the same instance. Concurrent unsynchronized read/write is a data race (go test -race will flag it) with undefined behavior. Lock the read-side getters (and IsActive) consistently, or snapshot the fields under a single lock.
🤖 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 `@internal/app/domain/server_task.go` around lines 219 - 241, Protect all
ServerTask field reads with s.mutex to match UpdateFromOptions: update
ServerID(), NodeID(), Command(), Server(), OverlapPolicy(), CatchupPolicy(),
Name(), Timezone(), Payload(), Enabled(), UpdatedAt(), and IsActive() to lock
consistently, preferably with defer unlock. Preserve each getter’s existing
return behavior while ensuring no listed field is read lock-free.
# Conflicts: # README.md # internal/app/grpc/file_handler.go # internal/app/server/files/files.go # internal/app/server/files/response.go # internal/app/server/server.go # internal/app/services/runner.go # internal/processmanager/shawl_windows.go # test/functional/server_tasks/suite_test.go # test/functional/servertest/files/chmod_test.go # test/functional/servertest/files/file_info_test.go # test/functional/servertest/files/list_test.go # test/functional/servertest/files/mkdir_test.go # test/functional/servertest/files/move_copy_test.go # test/functional/servertest/files/remove_test.go # test/functional/servertest/files/suite_test.go # test/functional/servertest/files/upload_download_test.go # test/functional/servertest/suite.go
Coverage Report for CI Build 29874148788Coverage decreased (-1.3%) to 36.195%Details
Uncovered Changes
Coverage Regressions142 previously-covered lines in 14 files lost coverage.
Coverage Stats
💛 - Coveralls |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/processmanager/shawl_windows.go (1)
426-430: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPass
sc.exeoptions and values as separate arguments.
sc.exe createrequires the equals sign to stay with the option name and the value to follow as a separate argument. These calls currently passstart=auto,obj=...,password=..., andbinPath=...as single args, which can cause service creation to fail or misparse.Also applies to
internal/processmanager/shawl_windows.go:454-459.🤖 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 `@internal/processmanager/shawl_windows.go` around lines 426 - 430, Update the scArgs construction in the service-creation flow and the corresponding block around the second sc.exe invocation so each sc.exe option and value are separate arguments: keep the equals sign with option names such as start=, obj=, password=, and binPath=, followed by their values as distinct elements. Preserve the existing option order and values while applying this consistently to both command argument lists.Source: MCP tools
🤖 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 `@internal/processmanager/shawl_windows.go`:
- Around line 466-470: The Debug calls in the service configuration logging
block expose complete command lines, credentials, and unescaped newlines. Remove
the logger.Debug calls for serviceConfig and binPath, or replace them with
non-sensitive redacted metadata that cannot include command contents or raw
newlines.
---
Outside diff comments:
In `@internal/processmanager/shawl_windows.go`:
- Around line 426-430: Update the scArgs construction in the service-creation
flow and the corresponding block around the second sc.exe invocation so each
sc.exe option and value are separate arguments: keep the equals sign with option
names such as start=, obj=, password=, and binPath=, followed by their values as
distinct elements. Preserve the existing option order and values while applying
this consistently to both command argument lists.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 9bf50c95-60f0-40bb-b129-67adc254adb0
📒 Files selected for processing (13)
config/gameap-daemon.yamlinternal/app/config/config.gointernal/app/config/config_test.gointernal/app/domain/server_task.gointernal/app/domain/server_task_test.gointernal/app/gdaemon_scheduler/const.gointernal/app/gdaemon_scheduler/task_manager.gointernal/app/gdaemon_scheduler/tasks_manager_test.gointernal/app/grpc/connection.gointernal/app/grpc/file_handler.gointernal/app/grpc/file_handler_test.gointernal/processmanager/shawl_windows.gointernal/processmanager/simple.go
🚧 Files skipped from review as they are similar to previous changes (6)
- internal/processmanager/simple.go
- internal/app/grpc/file_handler.go
- config/gameap-daemon.yaml
- internal/app/domain/server_task.go
- internal/app/gdaemon_scheduler/task_manager.go
- internal/app/config/config.go
Comment on lines
+466
to
+470
| // The service config and binPath embed the whole game command line, which may | ||
| // carry credentials passed as arguments. Task output is streamed to the panel, | ||
| // so both stay in the local daemon log only. | ||
| logger.Debug(ctx, "Service configuration: "+serviceConfig) | ||
| logger.Debug(ctx, "Service binPath: "+binPath) |
There was a problem hiding this comment.
Do not log the full command line or service configuration.
The previous task-output leak was removed, but logger.Debug now writes the same sensitive values to daemon logs. binPath and serviceConfig contain the complete game command and may include credentials; raw embedded newlines also permit log-forging output. Remove these logs or emit only redacted metadata.
Proposed fix
- // The service config and binPath embed the whole game command line, which may
- // carry credentials passed as arguments. Task output is streamed to the panel,
- // so both stay in the local daemon log only.
- logger.Debug(ctx, "Service configuration: "+serviceConfig)
- logger.Debug(ctx, "Service binPath: "+binPath)🧰 Tools
🪛 ast-grep (0.44.1)
[warning] 468-468: A log/format call (log.Print/Printf/Println, the Fatal/Panic variants, fmt.Sprintf, or a structured logger's Info/Warn/Error/Debug method) is given a message built by concatenating a string literal with a non-literal value such as request data. Unsanitized, attacker-controlled input written to logs enables log forging / CRLF injection: an attacker can inject newlines to spoof log entries or break log parsers. Do not concatenate raw input into the log message; pass it as a separate structured field/argument (e.g. 'log.Printf("user: %s", user)' or 'logger.Info("login", "user", user)') and strip or escape newline characters first.
Context: logger.Debug(ctx, "Service configuration: "+serviceConfig)
Note: [CWE-117] Improper Output Neutralization for Logs.
(log-injection-request-data-concat-go)
[warning] 469-469: A log/format call (log.Print/Printf/Println, the Fatal/Panic variants, fmt.Sprintf, or a structured logger's Info/Warn/Error/Debug method) is given a message built by concatenating a string literal with a non-literal value such as request data. Unsanitized, attacker-controlled input written to logs enables log forging / CRLF injection: an attacker can inject newlines to spoof log entries or break log parsers. Do not concatenate raw input into the log message; pass it as a separate structured field/argument (e.g. 'log.Printf("user: %s", user)' or 'logger.Info("login", "user", user)') and strip or escape newline characters first.
Context: logger.Debug(ctx, "Service binPath: "+binPath)
Note: [CWE-117] Improper Output Neutralization for Logs.
(log-injection-request-data-concat-go)
🤖 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 `@internal/processmanager/shawl_windows.go` around lines 466 - 470, The Debug
calls in the service configuration logging block expose complete command lines,
credentials, and unescaped newlines. Remove the logger.Debug calls for
serviceConfig and binPath, or replace them with non-sensitive redacted metadata
that cannot include command contents or raw newlines.
Source: Linters/SAST tools
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation