-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Execute debugger REPL commands inside job container #4420
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -860,6 +860,9 @@ internal async Task OnStepStartingAsync(IStep step, bool isFirstStep) | |
| // Send stopped event to debugger (only if client is connected) | ||
| SendStoppedEvent(reason, description); | ||
|
|
||
| // Emit a banner so the user knows where REPL commands will execute | ||
| SendExecutionContextBanner(step); | ||
|
|
||
| // Wait for debugger command | ||
| await WaitForCommandAsync(cancellationToken); | ||
| } | ||
|
|
@@ -1195,7 +1198,12 @@ private async Task<EvaluateResponseBody> DispatchReplCommandAsync( | |
|
|
||
| case RunCommand run: | ||
| var context = GetExecutionContextForFrame(frameId); | ||
| return await _replExecutor.ExecuteRunCommandAsync(run, context, cancellationToken); | ||
| bool isActionStep; | ||
| lock (_stateLock) | ||
| { | ||
| isActionStep = _currentStep is IActionRunner; | ||
| } | ||
| return await _replExecutor.ExecuteRunCommandAsync(run, context, isActionStep, cancellationToken); | ||
|
|
||
| default: | ||
| return new EvaluateResponseBody | ||
|
|
@@ -1407,6 +1415,40 @@ private void SendStoppedEvent(string reason, string description) | |
| }); | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Emits a console output banner telling the user whether REPL | ||
| /// commands will execute on the host or inside the job container. | ||
| /// </summary> | ||
| private void SendExecutionContextBanner(IStep step) | ||
| { | ||
| if (!_isClientConnected) | ||
| { | ||
| return; | ||
| } | ||
|
|
||
| bool isActionStep = step is IActionRunner; | ||
| var container = _jobContext?.Global?.Container; | ||
|
|
||
| string target; | ||
| if (isActionStep && container != null && | ||
| (!string.IsNullOrEmpty(container.ContainerId) || | ||
| FeatureManager.IsContainerHooksEnabled(_jobContext?.Global?.Variables))) | ||
|
Comment on lines
+1433
to
+1435
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. the condition might have some issue? |
||
| { | ||
| var image = container.ContainerImage ?? "container"; | ||
| var shortId = !string.IsNullOrEmpty(container.ContainerId) && container.ContainerId.Length >= 12 | ||
| ? container.ContainerId.Substring(0, 12) | ||
| : container.ContainerId ?? ""; | ||
| var idSuffix = !string.IsNullOrEmpty(shortId) ? $" ({shortId})" : ""; | ||
| target = $"job container: {image}{idSuffix}"; | ||
| } | ||
| else | ||
| { | ||
| target = "runner host"; | ||
| } | ||
|
|
||
| SendOutput("console", $"\nCommands will run on {target}\n"); | ||
| } | ||
|
|
||
| private string MaskUserVisibleText(string value) | ||
| { | ||
| if (string.IsNullOrEmpty(value)) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -9,6 +9,7 @@ | |
| using GitHub.Runner.Common; | ||
| using GitHub.Runner.Common.Util; | ||
| using GitHub.Runner.Sdk; | ||
| using GitHub.Runner.Worker.Container; | ||
| using GitHub.Runner.Worker.Handlers; | ||
|
|
||
| namespace GitHub.Runner.Worker.Dap | ||
|
|
@@ -43,6 +44,7 @@ public DapReplExecutor(IHostContext hostContext, Action<string, string> sendOutp | |
| public async Task<EvaluateResponseBody> ExecuteRunCommandAsync( | ||
| RunCommand command, | ||
| IExecutionContext context, | ||
| bool isActionStep, | ||
| CancellationToken cancellationToken) | ||
| { | ||
| if (context == null) | ||
|
|
@@ -52,7 +54,7 @@ public async Task<EvaluateResponseBody> ExecuteRunCommandAsync( | |
|
|
||
| try | ||
| { | ||
| return await ExecuteScriptAsync(command, context, cancellationToken); | ||
| return await ExecuteScriptAsync(command, context, isActionStep, cancellationToken); | ||
| } | ||
| catch (Exception ex) | ||
| { | ||
|
|
@@ -65,9 +67,17 @@ public async Task<EvaluateResponseBody> ExecuteRunCommandAsync( | |
| private async Task<EvaluateResponseBody> ExecuteScriptAsync( | ||
| RunCommand command, | ||
| IExecutionContext context, | ||
| bool isActionStep, | ||
| CancellationToken cancellationToken) | ||
| { | ||
| // 1. Resolve shell — same logic as ScriptHandler | ||
| // 1. Resolve step host — container or host, same as ActionRunner. | ||
| // Only action steps (user-defined run:/uses:) execute inside the | ||
| // container. Infrastructure steps (Set up job, Initialize | ||
| // containers, Complete job, etc.) always run on the host. | ||
| var stepHost = CreateStepHost(context, isActionStep); | ||
| var isContainerStepHost = stepHost is IContainerStepHost; | ||
|
|
||
| // 2. Resolve shell — same logic as ScriptHandler | ||
| string shellCommand; | ||
| string argFormat; | ||
|
|
||
|
|
@@ -87,9 +97,9 @@ private async Task<EvaluateResponseBody> ExecuteScriptAsync( | |
| argFormat = ScriptHandlerHelpers.GetScriptArgumentsFormat(shellCommand); | ||
| } | ||
|
|
||
| _trace.Info("Resolved REPL shell"); | ||
| _trace.Info($"Resolved REPL shell (container={isContainerStepHost})"); | ||
|
|
||
| // 2. Expand ${{ }} expressions in the script body, just like | ||
| // 3. Expand ${{ }} expressions in the script body, just like | ||
| // ActionRunner evaluates step inputs before ScriptHandler sees them | ||
| var contents = ExpandExpressions(command.Script, context); | ||
| contents = ScriptHandlerHelpers.FixUpScriptContents(shellCommand, contents); | ||
|
|
@@ -111,25 +121,44 @@ private async Task<EvaluateResponseBody> ExecuteScriptAsync( | |
|
|
||
| try | ||
| { | ||
| // 3. Format arguments with script path | ||
| var resolvedPath = scriptFilePath.Replace("\"", "\\\""); | ||
| // 4. Resolve script path — translate for container if needed | ||
| var resolvedPath = stepHost.ResolvePathForStepHost(context, scriptFilePath).Replace("\"", "\\\""); | ||
| if (string.IsNullOrEmpty(argFormat) || !argFormat.Contains("{0}")) | ||
| { | ||
| return ErrorResult($"Invalid shell option '{shellCommand}'. Shell must be a valid built-in (bash, sh, cmd, powershell, pwsh) or a format string containing '{{0}}'"); | ||
| } | ||
| var arguments = string.Format(argFormat, resolvedPath); | ||
|
|
||
| // 4. Resolve shell command path | ||
| // 5. Resolve shell command path — for containers, use the shell | ||
| // name directly (it will be resolved inside the container); | ||
| // for host execution, resolve the full path on the host. | ||
| string prependPath = string.Join( | ||
| Path.PathSeparator.ToString(), | ||
| Enumerable.Reverse(context.Global.PrependPath)); | ||
| var commandPath = WhichUtil.Which(shellCommand, false, _trace, prependPath) | ||
| ?? shellCommand; | ||
| var fileName = isContainerStepHost | ||
| ? shellCommand | ||
| : WhichUtil.Which(shellCommand, false, _trace, prependPath) ?? shellCommand; | ||
|
|
||
| // 5. Build environment — merge from execution context like a real step | ||
| // 6. Build environment — merge from execution context like a real step | ||
| var environment = BuildEnvironment(context, command.Env); | ||
|
|
||
| // 6. Resolve working directory | ||
| // 7. Handle PrependPath — mirrors Handler.AddPrependPathToEnvironment | ||
| if (context.Global.PrependPath.Count > 0) | ||
| { | ||
| if (stepHost is IContainerStepHost containerHost) | ||
| { | ||
| containerHost.PrependPath = prependPath; | ||
| } | ||
| else | ||
| { | ||
| string existingPath; | ||
| environment.TryGetValue(Constants.PathVariable, out existingPath); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. do we need to port everything? |
||
| existingPath = existingPath ?? System.Environment.GetEnvironmentVariable(Constants.PathVariable) ?? string.Empty; | ||
| environment[Constants.PathVariable] = PathUtil.PrependPath(prependPath, existingPath); | ||
| } | ||
| } | ||
|
|
||
| // 8. Resolve working directory — translate for container | ||
| var workingDirectory = command.WorkingDirectory; | ||
| if (string.IsNullOrEmpty(workingDirectory)) | ||
| { | ||
|
|
@@ -141,48 +170,58 @@ private async Task<EvaluateResponseBody> ExecuteScriptAsync( | |
| : null; | ||
| workingDirectory = workspace ?? _hostContext.GetDirectory(WellKnownDirectory.Work); | ||
| } | ||
| workingDirectory = stepHost.ResolvePathForStepHost(context, workingDirectory); | ||
|
|
||
| _trace.Info("Executing REPL command"); | ||
|
|
||
| // Stream execution info to debugger | ||
| SendOutput("console", $"$ {shellCommand} {command.Script.Substring(0, Math.Min(command.Script.Length, 80))}{(command.Script.Length > 80 ? "..." : "")}\n"); | ||
|
|
||
| // 7. Execute via IProcessInvoker (same as DefaultStepHost) | ||
| int exitCode; | ||
| using (var processInvoker = _hostContext.CreateService<IProcessInvoker>()) | ||
| // NOTE: When container hooks are enabled, ContainerStepHost routes | ||
| // execution through IContainerHookManager which does not raise | ||
| // OutputDataReceived/ErrorDataReceived events. Output will not be | ||
| // streamed to the debug console in that mode. | ||
| if (isContainerStepHost && FeatureManager.IsContainerHooksEnabled(context.Global?.Variables)) | ||
| { | ||
| _trace.Warning("Container hooks are enabled -- REPL output will not be streamed to the debug console"); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. where would end user see this? |
||
| } | ||
|
|
||
| // 9. Execute via IStepHost — handles docker exec for containers, | ||
| // direct process execution for host, and container hooks | ||
| stepHost.OutputDataReceived += (sender, args) => | ||
| { | ||
| processInvoker.OutputDataReceived += (sender, args) => | ||
| if (!string.IsNullOrEmpty(args.Data)) | ||
| { | ||
| if (!string.IsNullOrEmpty(args.Data)) | ||
| { | ||
| var masked = _hostContext.SecretMasker.MaskSecrets(args.Data); | ||
| SendOutput("stdout", masked + "\n"); | ||
| } | ||
| }; | ||
| var masked = _hostContext.SecretMasker.MaskSecrets(args.Data); | ||
| SendOutput("stdout", masked + "\n"); | ||
| } | ||
|
rentziass marked this conversation as resolved.
|
||
| }; | ||
|
|
||
| processInvoker.ErrorDataReceived += (sender, args) => | ||
| stepHost.ErrorDataReceived += (sender, args) => | ||
| { | ||
| if (!string.IsNullOrEmpty(args.Data)) | ||
| { | ||
| if (!string.IsNullOrEmpty(args.Data)) | ||
| { | ||
| var masked = _hostContext.SecretMasker.MaskSecrets(args.Data); | ||
| SendOutput("stderr", masked + "\n"); | ||
| } | ||
| }; | ||
|
|
||
| exitCode = await processInvoker.ExecuteAsync( | ||
| workingDirectory: workingDirectory, | ||
| fileName: commandPath, | ||
| arguments: arguments, | ||
| environment: environment, | ||
| requireExitCodeZero: false, | ||
| outputEncoding: null, | ||
| killProcessOnCancel: true, | ||
| cancellationToken: cancellationToken); | ||
| } | ||
| var masked = _hostContext.SecretMasker.MaskSecrets(args.Data); | ||
| SendOutput("stderr", masked + "\n"); | ||
| } | ||
| }; | ||
|
|
||
| int exitCode = await stepHost.ExecuteAsync( | ||
| context: context, | ||
| workingDirectory: workingDirectory, | ||
| fileName: fileName, | ||
| arguments: arguments, | ||
| environment: environment, | ||
| requireExitCodeZero: false, | ||
| outputEncoding: null, | ||
| killProcessOnCancel: true, | ||
| inheritConsoleHandler: false, | ||
| standardInInput: null, | ||
| cancellationToken: cancellationToken); | ||
|
|
||
| _trace.Info($"REPL command exited with code {exitCode}"); | ||
|
|
||
| // 8. Return only the exit code summary (output was already streamed) | ||
| // 10. Return only the exit code summary (output was already streamed) | ||
| return new EvaluateResponseBody | ||
| { | ||
| Result = exitCode == 0 ? $"(exit code: {exitCode})" : $"Process completed with exit code {exitCode}.", | ||
|
|
@@ -198,6 +237,43 @@ private async Task<EvaluateResponseBody> ExecuteScriptAsync( | |
| } | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Creates the appropriate <see cref="IStepHost"/> for the current | ||
| /// execution context, mirroring how <see cref="ActionRunner"/> decides | ||
| /// between host and container execution. | ||
| /// | ||
| /// Only action steps (user-defined run:/uses: steps) run inside the | ||
| /// job container. Infrastructure steps like "Set up job", "Initialize | ||
| /// containers", "Stop containers", and "Complete job" always execute | ||
| /// on the host regardless of whether a container is configured. | ||
| /// </summary> | ||
| internal IStepHost CreateStepHost(IExecutionContext context, bool isActionStep) | ||
| { | ||
| if (!isActionStep) | ||
| { | ||
| _trace.Info("Creating DefaultStepHost for REPL execution (infrastructure step)"); | ||
| return _hostContext.CreateService<IDefaultStepHost>(); | ||
| } | ||
|
|
||
| var container = context?.Global?.Container; | ||
| if (container != null) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. kind of similar question, when containerhook enabled for container step, and there is no job container, what would we use? |
||
| { | ||
| // Container hooks don't always set ContainerId, but the container | ||
| // step host handles that internally | ||
| var hooksEnabled = FeatureManager.IsContainerHooksEnabled(context.Global?.Variables); | ||
| if (hooksEnabled || !string.IsNullOrEmpty(container.ContainerId)) | ||
| { | ||
| _trace.Info("Creating ContainerStepHost for REPL execution"); | ||
| var containerStepHost = _hostContext.CreateService<IContainerStepHost>(); | ||
| containerStepHost.Container = container; | ||
| return containerStepHost; | ||
| } | ||
| } | ||
|
|
||
| _trace.Info("Creating DefaultStepHost for REPL execution"); | ||
| return _hostContext.CreateService<IDefaultStepHost>(); | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Expands <c>${{ }}</c> expressions in the input string using the | ||
| /// runner's template evaluator — the same evaluation path that processes | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we pass in
_currentStepsince that's the one captured under the lock?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
or just use
_currentStepand not pass in any arg?