Skip to content

ICM fixes (3/n) - #27925

Merged
Hariharan Seshadri (hariharans29) merged 4 commits into
mainfrom
hari/icm_3
Apr 9, 2026
Merged

ICM fixes (3/n)#27925
Hariharan Seshadri (hariharans29) merged 4 commits into
mainfrom
hari/icm_3

Conversation

@hariharans29

Copy link
Copy Markdown
Member

Description

Fix ICM issue: https://portal.microsofticm.com/imp/v5/incidents/details/31000000567822/summary

The ICM is mainly about 2 issues in validate_package.py which was fixed by #27840.
But the ICM also references another issue in whisper_jump_times.py which is what this PR fixes

Motivation and Context

ICM fixes

Copilot AI 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.

Pull request overview

This PR addresses an additional security-related ICM item in the Whisper tooling by removing a shell-based command execution path in whisper_jump_times.py, aligning it with the safer approach taken in the earlier validate_package.py fix.

Changes:

  • Replace os.system("pip install ninja") with subprocess.run([...], check=True) to avoid shell invocation.
  • Invoke pip via sys.executable -m pip for a more reliable interpreter/environment match.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@tianleiwu Tianlei Wu (tianleiwu) 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.

No findings from my review. Switching the fallback from os.system("pip install ninja") to subprocess.run([sys.executable, "-m", "pip", "install", "ninja"], check=True) removes shell invocation, uses the active interpreter consistently, and makes installation failures explicit.

@hariharans29
Hariharan Seshadri (hariharans29) merged commit 3fad293 into main Apr 9, 2026
108 of 111 checks passed
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.

3 participants