Conversation
Package a dependency-free Python compatibility surface for existing JobSender consumers and stage result uploads for the AOT Helix client. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 1878358f-1e80-41bf-a8d5-4b3d0e13d7f9
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
There are correctness/resiliency issues in the new compatibility logging/test execution code that should be addressed before approval.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
This PR adds a dependency-free Python compatibility shim that can be packaged with Microsoft.DotNet.Helix.JobSender to keep legacy scriptrunner.py payloads working on newer Helix queues that no longer install the legacy helix Python package. The shim stages logs and result artifacts under HELIX_WORKITEM_UPLOAD_ROOT so the machine-side Helix client remains responsible for credentials and uploads.
Changes:
- Package legacy-runner Python compatibility modules (namespace-package style, no
helix/__init__.py) and a build-transitive targets file to copy them into consuming outputs underRunnerScripts/scriptrunner/helix. - Implement minimal
helix.*runner surfaces (logging, process execution, cmdline parsing, staging “uploads” toHELIX_WORKITEM_UPLOAD_ROOT). - Add tests (Python unittest + C# process-driven runner) validating import surface and staging behavior; document the compatibility shim and explicitly exclude
continuationrunner.py.
File summaries
| File | Description |
|---|---|
| src/Microsoft.DotNet.Helix/JobSender/Readme.md | Documents the legacy runner shim behavior, limits, and opt-out property. |
| src/Microsoft.DotNet.Helix/JobSender/Microsoft.DotNet.Helix.JobSender.csproj | Packs the shim files and buildTransitive targets into the JobSender NuGet. |
| src/Microsoft.DotNet.Helix/JobSender/legacy-runner/README.md | Adds a focused README describing what the shim is and when it can be removed. |
| src/Microsoft.DotNet.Helix/JobSender/legacy-runner/helix/settings.py | Provides environment-backed settings used by legacy runner code paths. |
| src/Microsoft.DotNet.Helix/JobSender/legacy-runner/helix/saferequests.py | Adds an import-compatible placeholder for unused safe-requests functionality. |
| src/Microsoft.DotNet.Helix/JobSender/legacy-runner/helix/proc.py | Implements minimal subprocess execution with output logging. |
| src/Microsoft.DotNet.Helix/JobSender/legacy-runner/helix/logs.py | Implements logging configuration + staged log file behavior under upload root. |
| src/Microsoft.DotNet.Helix/JobSender/legacy-runner/helix/helixio.py | Adds basic file/path helpers and directory copy utilities. |
| src/Microsoft.DotNet.Helix/JobSender/legacy-runner/helix/cmdline.py | Implements legacy-style command-line parsing and settings overrides. |
| src/Microsoft.DotNet.Helix/JobSender/legacy-runner/helix/azure_storage.py | Replaces direct uploads with staging under HELIX_WORKITEM_UPLOAD_ROOT. |
| src/Microsoft.DotNet.Helix/JobSender/legacy-runner/helix_test_execution.py | Provides compatibility HelixTestExecution used by AppCompat runners. |
| src/Microsoft.DotNet.Helix/JobSender/buildTransitive/Microsoft.DotNet.Helix.JobSender.targets | Copies shim files into consuming project output/publish under legacy runner paths. |
| src/Microsoft.DotNet.Helix/JobSender.Tests/Microsoft.DotNet.Helix.JobSender.Tests.csproj | Adds shim files + test runner script to test output for execution. |
| src/Microsoft.DotNet.Helix/JobSender.Tests/LegacyRunnerCompatTests.py | Adds Python behavior tests validating staging, logging, and import surface. |
| src/Microsoft.DotNet.Helix/JobSender.Tests/LegacyRunnerCompatProcessTests.cs | Runs the Python behavior tests from xUnit as part of the test suite. |
| src/Microsoft.DotNet.Helix/JobSender.Tests/LegacyRunner/scriptrunner.py | Provides a legacy-shaped runner script used by the compatibility tests. |
Review details
Suppressed comments (1)
src/Microsoft.DotNet.Helix/JobSender/legacy-runner/helix/logs.py:51
- set_logfile() adds a FileHandler without ensuring the destination directory exists. If HELIX_LOG_ROOT points to a non-existent directory, this will raise and prevent the runner from emitting any results/logs.
def set_logfile(path):
_configure()
_add_handler(logging.FileHandler(path))
- Files reviewed: 16/16 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
The Python compatibility tests inherited the ambient Helix work-item environment, so on a legacy queue the child imported the installed helix package (which wins over this namespace shim) and uploaded using real result-container tokens instead of staging. Child processes now run with a sanitized environment and skip with an explicit reason when an installed helix takes precedence. Also emit true UTC log timestamps, create the log directory in set_logfile, and make the xUnit wrapper fall back to another interpreter name and time out instead of hanging. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 1878358f-1e80-41bf-a8d5-4b3d0e13d7f9
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
The new xUnit test fails outright when Python isn’t installed, making the test suite environment-dependent.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
src/Microsoft.DotNet.Helix/JobSender.Tests/LegacyRunnerCompatProcessTests.cs:84
- This test hard-fails when no Python interpreter is present, which can make
dotnet testfor the repo environment-dependent (e.g., developer machines or constrained CI images). Consider skipping the test when Python isn't available, since the compatibility behavior can only be validated when an interpreter exists.
- Files reviewed: 16/16 changed files
- Comments generated: 0 new
- Review effort level: Lite
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
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
Microsoft.DotNet.Helix.JobSenderRunnerScripts/scriptrunner/scriptrunner.pythrough a build-transitive targetHELIX_WORKITEM_UPLOAD_ROOTso the machine-side Helix client retains ownership of credentials and uploadshelix/__init__.pycontinuationrunner.pyMotivation
New AOT Helix queues no longer install the legacy Python
helixpackage. Existing JobSender consumers can still submit payloads that invokescriptrunner.py, causingModuleNotFoundErrorbefore their tests run and preventing their expected script logs andTestResults.zipfrom being published.This keeps public Arcade independent from the internal machine-client implementation. The shim only implements the submitted runner surface; the Helix client continues to own storage authentication, retries, uploads, and event publication.
Validation
dotnet test src/Microsoft.DotNet.Helix/JobSender.Tests/Microsoft.DotNet.Helix.JobSender.Tests.csproj --no-restore— 6 passedscriptrunner.pyshape end to end with a syntheticRunMstest.cmdTestResults.zipis staged underHELIX_WORKITEM_UPLOAD_ROOTMicrosoft.DotNet.Helix.JobSender.12.0.0-devand verified all compatibility files are presentRunnerScripts/scriptrunnerFollow-up
The AOT client change that emits legacy
Logsevents for staged.helix-logsfiles and fails work items on upload/event-publication failures is being handled indotnet-helix-machines.