github/actions-runner
1
0
Fork 0
mirror of https://github.com/actions/runner.git synced 2026-07-03 11:06:08 +08:00
Code Issues Packages Projects Releases Wiki Activity

Address Copilot review feedback

Browse Source 
- Remove unused `using System.Linq;` from JobExecutionViewRendererL0
    and `using System.Collections.Generic;` from StepEntryTranslatorL0.
  - StepEntryTranslator: read run-step inputs through
    `PipelineConstants.ScriptStepInputs.*` (`script`, `shell`,
    `workingDirectory`). The runner stores these keys in their
    constants-defined spelling — see ActionManifestManagerWrapper:244 —
    not their kebab-case workflow-YAML form, so the previous
    `working-directory` lookup never matched in practice. Tests
    updated to use the same constants.
  - TemplateTokenYamlAdapter / JobExecutionViewRenderer.FormatScalar:
    defensively also strip a `---\n` prefix on top of the existing
    `--- ` prefix. Not observed in any input I exercised, but cheap
    insurance against an Emitter quirk on some YAML configurations.
  - JobExecutionView.Append: reject passing both `stepIdentity` and
    `matchKey`. The combination would orphan the original step's
    line mapping the moment TryClaim overwrites `_stepIdentities`
    for a different step. Add an L0 test covering the precondition.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This commit is contained in:
Francesco Renzi
2026-05-13 04:22:59 -07:00
committed by GitHub
parent 79e0e5cbe6
commit c72457ad4a
7 changed files with 69 additions and 28 deletions
Download Patch File Download Diff File Expand all files Collapse all files

23
src/Runner.Worker/Dap/JobExecutionView.cs
View File

@@ -125,20 +125,25 @@ namespace GitHub.Runner.Worker.Dap
}
/// <summary>
/// Append a new entry. If <paramref name="stepIdentity"/> is non-null,
/// registers the IStep -> line mapping for later lookup. If
/// <paramref name="matchKey"/> is non-null, the entry is registered
/// as an unclaimed placeholder that a future
/// <see cref="TryClaim(string, IStep)"/> call can bind to a real
/// IStep (used by the predictive Post-step path). Re-renders the
/// YAML and updates the start-line table.
/// Append a new entry. Exactly one of <paramref name="stepIdentity"/>
/// or <paramref name="matchKey"/> may be non-null (or both may be
/// null for a static entry that needs no line lookup):
/// - <paramref name="stepIdentity"/> non-null: registers the
/// IStep→line mapping immediately. Use when the real
/// <see cref="IStep"/> is known at append time.
/// - <paramref name="matchKey"/> non-null: registers an unclaimed
/// placeholder that a later <see cref="TryClaim"/> binds to a
/// real <see cref="IStep"/>.
/// Re-renders the YAML and updates the start-line table.
/// </summary>
/// <returns>1-based line number of the newly-appended entry's <c>- step:</c> key.</returns>
public int Append(JobExecutionViewEntry entry, IStep stepIdentity = null, string matchKey = null)
{
if (entry == null)
ArgUtil.NotNull(entry, nameof(entry));
if (stepIdentity != null && matchKey != null)
{
throw new ArgumentNullException(nameof(entry));
throw new ArgumentException(
"Append cannot register both a step identity and a placeholder match key on the same entry; pass at most one.");
}
lock (_lock)

7
src/Runner.Worker/Dap/JobExecutionViewRenderer.cs
View File

@@ -366,10 +366,17 @@ namespace GitHub.Runner.Worker.Dap
emitter.Emit(new StreamEnd());
string raw = sw.ToString();
// Strip YAML document markers. Emitter elides these for most
// scalars but emits "--- " (with space) for some edge cases
// (e.g. empty strings). Defensively handle "---\n" too.
if (raw.StartsWith("--- ", StringComparison.Ordinal))
{
raw = raw.Substring(4);
}
else if (raw.StartsWith("---\n", StringComparison.Ordinal))
{
raw = raw.Substring(4);
}
raw = raw.TrimEnd('\n');
const string DocEndMarker = "\n...";
if (raw.EndsWith(DocEndMarker, StringComparison.Ordinal))

16
src/Runner.Worker/Dap/StepEntryTranslator.cs
View File

@@ -16,12 +16,14 @@ namespace GitHub.Runner.Worker.Dap
internal static class StepEntryTranslator
{
// Run-step internals carried on ActionStep.Inputs that are NOT
// user-authored `with:` entries.
// user-authored `with:` entries. The runner stores these under
// the keys defined in PipelineConstants.ScriptStepInputs, NOT
// their kebab-case workflow-YAML spellings.
private static readonly HashSet<string> RunStepInternalKeys = new(StringComparer.Ordinal)
{
"script",
"shell",
"working-directory",
PipelineConstants.ScriptStepInputs.Script,
PipelineConstants.ScriptStepInputs.Shell,
PipelineConstants.ScriptStepInputs.WorkingDirectory,
};
/// <summary>
@@ -116,11 +118,11 @@ namespace GitHub.Runner.Worker.Dap
var inputs = action.Inputs as MappingToken;
if (inputs != null)
{
if (TryGetMapValue(inputs, "script", out var scriptTok) && scriptTok != null)
if (TryGetMapValue(inputs, PipelineConstants.ScriptStepInputs.Script, out var scriptTok) && scriptTok != null)
{
run = scriptTok.ToString();
}
if (TryGetMapValue(inputs, "shell", out var shellTok) && shellTok != null)
if (TryGetMapValue(inputs, PipelineConstants.ScriptStepInputs.Shell, out var shellTok) && shellTok != null)
{
string shellText = shellTok.ToString();
if (!string.IsNullOrEmpty(shellText))
@@ -128,7 +130,7 @@ namespace GitHub.Runner.Worker.Dap
shell = shellText;
}
}
if (TryGetMapValue(inputs, "working-directory", out var wdTok) && wdTok != null)
if (TryGetMapValue(inputs, PipelineConstants.ScriptStepInputs.WorkingDirectory, out var wdTok) && wdTok != null)
{
string wdText = wdTok.ToString();
if (!string.IsNullOrEmpty(wdText))

11
src/Runner.Worker/Dap/TemplateTokenYamlAdapter.cs
View File

@@ -107,11 +107,20 @@ namespace GitHub.Runner.Worker.Dap
adapter.WriteEnd();
string raw = sw.ToString();
// Strip YAML document markers ("--- " prefix and "\n..." suffix).
// Strip YAML document markers. The Emitter most commonly elides
// these for our use (DocumentStart isImplicit=true), but emits
// them for some scalar edge cases (e.g. empty strings) and may
// emit them on their own line for collection roots under some
// settings. Strip both shapes defensively so callers never see
// a leaked marker leak into the embedded fragment.
if (raw.StartsWith("--- ", StringComparison.Ordinal))
{
raw = raw.Substring(4);
}
else if (raw.StartsWith("---\n", StringComparison.Ordinal))
{
raw = raw.Substring(4);
}
const string DocEndMarker = "\n...";
if (raw.EndsWith(DocEndMarker + "\n", StringComparison.Ordinal))
{

16
src/Test/L0/Worker/JobExecutionViewL0.cs
View File

@@ -422,5 +422,21 @@ namespace GitHub.Runner.Common.Tests.Worker
Assert.Contains(line.Value, entryLines);
}
}
[Fact]
[Trait("Level", "L0")]
[Trait("Category", "Worker")]
public void Append_RejectsBothStepIdentityAndMatchKey()
{
// Allowing both would orphan the IStep→line mapping the moment
// TryClaim overwrites _stepIdentities[index] for a different
// step, so the API rejects the combination at append time.
var view = new JobExecutionView("j");
var entry = new JobExecutionViewEntry(JobExecutionPhase.Post, "Post X", uses: "actions/x@v1");
Assert.Throws<ArgumentException>(() =>
view.Append(entry, stepIdentity: NewStep("real"), matchKey: "k1"));
// State unchanged.
Assert.Equal(0, view.EntryCount);
}
}
}

1
src/Test/L0/Worker/JobExecutionViewRendererL0.cs
View File

@@ -1,6 +1,5 @@
using System;
using System.Collections.Generic;
using System.Linq;
using GitHub.Runner.Worker.Dap;
using Xunit;

23
src/Test/L0/Worker/StepEntryTranslatorL0.cs
View File

@@ -1,5 +1,4 @@
using System;
using System.Collections.Generic;
using GitHub.DistributedTask.ObjectTemplating.Tokens;
using GitHub.DistributedTask.Pipelines;
using GitHub.Runner.Worker;
@@ -244,13 +243,17 @@ namespace GitHub.Runner.Common.Tests.Worker
[Trait("Category", "Worker")]
public void Translate_RunStep_ExtractsShellAndWorkingDirectory()
{
// The runner stores run-step inputs under the keys defined in
// PipelineConstants.ScriptStepInputs (camelCase), NOT their
// kebab-case workflow-YAML spellings — see
// ActionManifestManagerWrapper:244.
var action = new ActionStep
{
Reference = new ScriptReference(),
Inputs = Map(
("script", Str("npm test")),
("shell", Str("bash")),
("working-directory", Str("./api"))),
(PipelineConstants.ScriptStepInputs.Script, Str("npm test")),
(PipelineConstants.ScriptStepInputs.Shell, Str("bash")),
(PipelineConstants.ScriptStepInputs.WorkingDirectory, Str("./api"))),
};
var mock = NewActionRunnerMock(ActionRunStage.Main, "Run", new ScriptReference(), action);
@@ -276,9 +279,9 @@ namespace GitHub.Runner.Common.Tests.Worker
Reference = reference,
Inputs = Map(
("mode", Str("ci")),
("script", Str("leak")),
("shell", Str("leak")),
("working-directory", Str("leak"))),
(PipelineConstants.ScriptStepInputs.Script, Str("leak")),
(PipelineConstants.ScriptStepInputs.Shell, Str("leak")),
(PipelineConstants.ScriptStepInputs.WorkingDirectory, Str("leak"))),
};
var mock = NewActionRunnerMock(ActionRunStage.Main, "Run", reference, action);
@@ -288,9 +291,9 @@ namespace GitHub.Runner.Common.Tests.Worker
Assert.NotNull(entry.WithYaml);
Assert.Contains("mode: ci", entry.WithYaml);
Assert.DoesNotContain("leak", entry.WithYaml);
Assert.DoesNotContain("script", entry.WithYaml);
Assert.DoesNotContain("shell", entry.WithYaml);
Assert.DoesNotContain("working-directory", entry.WithYaml);
Assert.DoesNotContain(PipelineConstants.ScriptStepInputs.Script, entry.WithYaml);
Assert.DoesNotContain(PipelineConstants.ScriptStepInputs.Shell, entry.WithYaml);
Assert.DoesNotContain(PipelineConstants.ScriptStepInputs.WorkingDirectory, entry.WithYaml);
}
[Fact]