More robust path substitution for container action expressions

I haven't used C# in almost 15 years, so take what I'm about to say with a grain of salt. But as best as I can tell, GitHub.Runner.Worker.Container.ContainerInfo#TranslateToContainerPath is being used here and here to translate host paths to container paths for inputs to container actions. However, the way it's implemented does a very simple string search that only checks the beginning of the value for the translatable paths. I'm guessing it's to avoid false positives. However, in my case, it's leading to false negatives.

I've tried doing this for one of my actions in order to pass multiple file paths in one input:

name: 'Work with files'
on:
  push:
    branches:
      - 'main'
  workflow_dispatch:
jobs:
  deploy:
    runs-on: 'ubuntu-latest'
    steps:
      - name: 'Checkout repo'
        uses: 'actions/checkout@v2'

      - name: 'Container action'
        uses: './.github/actions/container-action'
        with:
          files: |
            ${{ github.workspace }}/file1
            ${{ github.workspace }}/file2

However, because of when and how TranslateToContainerPath runs, the value of INPUT_FILES for example ends up only having the first file path translated:

INPUT_FILES=/github/workspace/file1
/home/runner/work/repo/repo/file2

Then, when I try to access those files, file2 is not found since that path doesn't exist within the container.

The path I would take to remedy this would be to modify the body of the GitHub.Runner.Worker.Handlers.ContainerActionHandler#RunAsync function. First, I'd move the path translations setup to occur before the template evaluation. Next, after the setup of the path translations but before the template evaluation, I'd loop over the parts of the ExecutionContext I know may need to be updated and run container.TranslateToContainerPath on them. I'd add them to extraExpressionValues since those seem to override the ExecutionContext.ExpressionValues in a given call to EvaluateContainerArguments without permanently altering the context values.

Maybe something like this:

var githubContext = new DictionaryContextData();
foreach (var variable in ExecutionContext.ExpressionValues["github"] as GitHubContext)
{
    translatedGithubContext.Add(variable.Key, new StringContextData(container.TranslateToContainerPath(variable.Value)));
}

// ... snip ...

extraExpressionValues["github"] = translatedGithubContext;

(Apologies for any errors in there. I'm sure I got something wrong or missed a null check or something. Like I said, my C# is very rusty.)

Theoretically, I think these path translations could be done completely in this way instead of going the existing route where the paths are updated after the templates have been evaluated. Although in order to maintain compatibility, user-entered strings would have to be translated as well. Probably still after the templates have been evaluated to make sure no template expressions are modified by the translator.

Here's what I mean. Let's say we have this action.yml:

name: 'Work with files'
on:
  push:
    branches:
      - 'main'
  workflow_dispatch:
jobs:
  deploy:
    runs-on: 'ubuntu-latest'
    steps:
      - name: 'Checkout repo'
        uses: 'actions/checkout@v2'

      - name: 'Make some output'
        id: 'some-other-step
        runs: 'echo "::set-output name=thing::/home/runner/work/repo/repo"'

      - name: 'Container action'
        uses: './.github/actions/container-action'
        with:
          one: '${{ github.workspace }}/file1'
          two: |
            ${{ github.workspace }}/file2.1
            ${{ github.workspace }}/file2.2
          three: '/home/runner/work/repo/repo/file3'
          four: '${{ steps.some-other-step.outputs.thing == github.workspace }}'
          five: '${{ "/home/runner/work/repo/repo" == github.workspace }}'

I would want INPUT_ONE to translate to /github/workspace/file1 and INPUT_TWO to translate to /github/workspace/file2.1\n/github/workspace/file2.2.

But then things get a little weird. With the runner's current behavior, INPUT_THREE would be translated to /github/workspace/file3. Is that the desired behavior? With the runner's current behavior, INPUT_FOUR and INPUT_FIVE should evaluate to true. (I think. I haven't tested it. I've only read the code. 😝) Is that the desired behavior? I don't know. I could see making an argument for INPUT_THREE to remain /home/runner/work/repo/repo/file3 since that's a literal value. INPUT_FOUR and INPUT_FIVE are a bit more ambiguous IMHO.

Just some things to think about, both how this feature could be improved, and some potential pitfalls in doing so. Please let me know if you have any questions or suggestions on how I can help.

actions / runner

More robust path substitution for container action expressions #1174