Add SubprocessPlugin for subprocess.run and shutil.which interception#1
Add SubprocessPlugin for subprocess.run and shutil.which interception#1
Conversation
…eption
Adds a new bigfoot plugin that intercepts subprocess.run and shutil.which
at the module level using reference-counted class-level activation,
following the HttpPlugin pattern exactly.
Key behaviors:
- mock_run: strict FIFO queue - wrong command or empty queue raises
UnmockedInteractionError immediately (bouncer guarantee)
- mock_which: semi-permissive - unregistered names return None silently;
registered names tracked on timeline with required=False default
- assertable_fields: only {"command"} for run, {"name"} for which
- install(): no-op method to trigger proxy creation before sandbox entry
for tests that want bouncer active with no mocks registered
Adds subprocess_mock singleton proxy to bigfoot.__init__ following the
same lazy-creation pattern as the existing http singleton.
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new SubprocessPlugin for intercepting subprocess.run and shutil.which, along with comprehensive documentation and tests. The implementation is solid and follows the existing patterns in the library. I've found a couple of edge cases that are not handled correctly and suggested some minor refactorings for conciseness. Overall, this is a great addition to the library.
| def _which_interceptor(name: str, **kwargs: Any) -> str | None: # noqa: ANN401 | ||
| verifier = _get_verifier_or_raise(_SOURCE_WHICH) | ||
| plugin = _find_subprocess_plugin(verifier) | ||
| return plugin._handle_which(name) |
There was a problem hiding this comment.
The _which_interceptor correctly accepts extra keyword arguments via **kwargs, but it fails to pass them to plugin._handle_which(name). This means that if shutil.which is called with arguments like mode or path, they will be dropped.
To fix this, you should pass **kwargs to _handle_which and also update the signature of _handle_which at line 367 to accept them (e.g., def _handle_which(self, name: str, **kwargs: Any) -> str | None:).
| def _which_interceptor(name: str, **kwargs: Any) -> str | None: # noqa: ANN401 | |
| verifier = _get_verifier_or_raise(_SOURCE_WHICH) | |
| plugin = _find_subprocess_plugin(verifier) | |
| return plugin._handle_which(name) | |
| def _which_interceptor(name: str, **kwargs: Any) -> str | None: # noqa: ANN401 | |
| verifier = _get_verifier_or_raise(_SOURCE_WHICH) | |
| plugin = _find_subprocess_plugin(verifier) | |
| return plugin._handle_which(name, **kwargs) |
| if args: | ||
| cmd = args[0] | ||
| else: | ||
| cmd = kwargs.get("args", []) | ||
| cmd_list = list(cmd) |
There was a problem hiding this comment.
The _handle_run method does not correctly handle the case where subprocess.run is called with a command as a string instead of a list of strings. The line cmd_list = list(cmd) will split a string command into a list of characters, which will then fail to match any mock configured with a list of strings. This can lead to confusing UnmockedInteractionErrors.
For example, subprocess.run("ls") would result in cmd_list being ['l', 's'], which would not match a mock like mock_run(["ls"]).
I suggest adding a check to explicitly disallow string commands. This would make the behavior clearer than the current silent mis-handling.
if args:
cmd = args[0]
else:
cmd = kwargs.get("args", [])
if isinstance(cmd, str):
raise TypeError("bigfoot.subprocess_mock only supports list-of-strings commands, not a single string.")
cmd_list = list(cmd)| for p in verifier._plugins: | ||
| if isinstance(p, _SubprocessPlugin): | ||
| plugin = p | ||
| break |
There was a problem hiding this comment.
This loop to find the plugin can be written more concisely using next() with a generator expression. This can improve readability and is a common Python idiom.
plugin = next((p for p in verifier._plugins if isinstance(p, _SubprocessPlugin)), None)| for plugin in verifier._plugins: | ||
| if isinstance(plugin, SubprocessPlugin): | ||
| return plugin | ||
| raise RuntimeError( | ||
| "BUG: bigfoot SubprocessPlugin interceptor is active but no " | ||
| "SubprocessPlugin is registered on the current verifier." | ||
| ) |
There was a problem hiding this comment.
This loop and subsequent raise can be simplified by using next() within a try...except StopIteration block. This is a more concise and idiomatic way to find the first item in a sequence or raise an error if it's not found.
| for plugin in verifier._plugins: | |
| if isinstance(plugin, SubprocessPlugin): | |
| return plugin | |
| raise RuntimeError( | |
| "BUG: bigfoot SubprocessPlugin interceptor is active but no " | |
| "SubprocessPlugin is registered on the current verifier." | |
| ) | |
| try: | |
| return next(p for p in verifier._plugins if isinstance(p, SubprocessPlugin)) | |
| except StopIteration: | |
| raise RuntimeError( | |
| "BUG: bigfoot SubprocessPlugin interceptor is active but no " | |
| "SubprocessPlugin is registered on the current verifier." | |
| ) from None |
- Remove quoted type annotation in assert_interaction (UP037) - Use cast() in format_unused_mock_hint to properly type tuple destructuring - Rename which-loop variable to avoid type collision with run-loop config - Remove unused type: ignore[assignment] comments on subprocess.run assignments - Shorten format_assert_hint lines via sm variable (E501) - Remove unused imports from test file (F401) - Add noqa: ANN401 to _handle_run for Any-typed *args/**kwargs
- SubprocessPlugin: pass **kwargs through _which_interceptor to _handle_which (shutil.which mode/path args were silently dropped) - SubprocessPlugin: raise TypeError on string commands in _handle_run instead of silently splitting into characters - Replace loop-and-break plugin lookups with idiomatic next() in subprocess, popen, and proxy code - Extract _push_cm() helper in _BigfootModule to deduplicate __enter__/__aenter__ sandbox creation logic Bump version to 0.10.1.
* fix: address Gemini Code Assist review feedback from PRs #1-3 - SubprocessPlugin: pass **kwargs through _which_interceptor to _handle_which (shutil.which mode/path args were silently dropped) - SubprocessPlugin: raise TypeError on string commands in _handle_run instead of silently splitting into characters - Replace loop-and-break plugin lookups with idiomatic next() in subprocess, popen, and proxy code - Extract _push_cm() helper in _BigfootModule to deduplicate __enter__/__aenter__ sandbox creation logic Bump version to 0.10.1. * fix: resolve 14 mypy strict-mode errors - Add explicit type annotations to avoid Returning Any errors - Remove unused type: ignore comments - Add import-untyped ignores for psycopg2 and asyncpg (no stubs available) - Add assignment ignores for aiohttp URL vs str type mismatch
1 participant
Summary
SubprocessPluginthat interceptssubprocess.runandshutil.whichglobally during a bigfoot sandbox using class-level reference counting (compatible with nested sandboxes)bigfoot.subprocess_mockmodule-level proxy that auto-createsSubprocessPluginon the current test verifier on first accesssubprocess.runmocks: calls must match in registration order; mismatched or extra calls raiseUnmockedInteractionErrorshutil.whichmocking: unregistered names returnNonesilently; registered names are tracked on the timelineConflictErrorifsubprocess.runorshutil.whichis already patched by a third party (e.g.,unittest.mock,pytest-mock) when the bouncer activatesNew API
subprocess_mock.mock_run(command, *, returncode, stdout, stderr, raises, required)subprocess_mock.mock_which(name, returns, *, required=False)subprocess_mock.install()— activates the bouncer without registering any mockssubprocess_mock.run/subprocess_mock.which— sentinels forassert_interaction()Test plan
Changes
src/bigfoot/plugins/subprocess.py— new pluginsrc/bigfoot/__init__.py— exportssubprocess_mock, addsSubprocessPluginto__all__tests/unit/test_init.py— updated__all__assertiondocs/— new guide and reference pages, updated index/navCHANGELOG.md— v0.3.0 entrypyproject.toml— version bump 0.2.0 → 0.3.0🤖 Generated with Claude Code