Skip to content

fix(sandbox): suppress policy warnings during initial workflow load - #1690

Open
Sakshamm-Goyal wants to merge 4 commits into
temporalio:mainfrom
Sakshamm-Goyal:agent/fix-sandbox-initial-import-warning
Open

fix(sandbox): suppress policy warnings during initial workflow load#1690
Sakshamm-Goyal wants to merge 4 commits into
temporalio:mainfrom
Sakshamm-Goyal:agent/fix-sandbox-initial-import-warning

Conversation

@Sakshamm-Goyal

@Sakshamm-Goyal Sakshamm-Goyal commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

Summary

  • suppress import-notification policy only for the sandbox’s initial workflow-module import
  • retain configured warning and raise behavior for that module’s import-time dependencies
  • add regressions covering both sides of that split and a high-level changelog entry

Validation

  • focused initial-module/dependency plus warning/error sandbox tests (4 passed)
  • Ruff check and format check
  • Pyright (0 errors)

Fixes #1254.

@Sakshamm-Goyal
Sakshamm-Goyal requested a review from a team as a code owner July 29, 2026 16:06
@VegetarianOrc

VegetarianOrc commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

Hey @Sakshamm-Goyal, thanks for taking a look at this!

The fix isn't quite correct as the import time dependencies of the workflow module are intended to trigger the warnings/exceptions. Those dependencies are loaded repeatedly when workflows are executed in the sandbox and the warnings are intended to highlight that those dependencies might be worth explicitly passing through to share them across workflow instances and avoid the processing/memory overhead of reloading them.

Addressing this issue would need to be scoped to suppressing the warnings for the imported workflow module only and allow the import time dependencies of that module to trigger configured the warnings or exceptions.

@Sakshamm-Goyal

Sakshamm-Goyal commented Jul 29, 2026

Copy link
Copy Markdown
Contributor Author

Thanks — narrowed the suppression to the initial workflow module only in commit fb1b110.

Import-time dependencies now continue through the configured notification policy, including both warning and raise modes. Added regression coverage for the workflow-module/dependency split; focused sandbox tests (4) plus Ruff and Pyright pass locally. CI is rerunning on the updated head.

@VegetarianOrc VegetarianOrc left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The approach looks okay to me, but the new check should be more narrowly applied.

Also please update the PR description to reflect the new behavior and include a changelog entry describing the new behavior.

Comment thread temporalio/worker/workflow_sandbox/_importer.py
@Sakshamm-Goyal

Copy link
Copy Markdown
Contributor Author

Addressed in 5788b9f. The initial-module condition now applies only inside the warning/raise notification block, so the normal importer flow is unchanged. I also updated the PR description and added the Unreleased changelog entry. Focused sandbox tests (4), Ruff, and Pyright remain green; CI is rerunning.

@Sakshamm-Goyal
Sakshamm-Goyal force-pushed the agent/fix-sandbox-initial-import-warning branch from 5788b9f to de6e055 Compare July 30, 2026 19:28
@Sakshamm-Goyal

Copy link
Copy Markdown
Contributor Author

The failed lint matrix was caused by Ruff I001 in the added fixture module, not the sandbox behavior. I reordered that module’s imports only and force-pushed the amended commit (de6e055). Local validation passed: Ruff import/full checks, Ruff format check, Pyright, and the two focused initial-import sandbox tests. Fresh CI is now running.

@Sakshamm-Goyal

Sakshamm-Goyal commented Jul 30, 2026

Copy link
Copy Markdown
Contributor Author

Correction to the CI status: the new fork-PR workflows entered action_required with zero jobs, so they need upstream approval before the matrix can run. CODEOWNERS and CLA are already green. The local validation reported above remains clean; I will monitor the approved rerun rather than attempting to bypass this gate.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug] SandboxImportNotificationPolicy.WARN_ON_UNINTENTIONAL_PASSTHROUGH warns on import of the workflow itself to the sandbox

2 participants