📝 CodeRabbit Chat: Implement requested code changes - #126
📝 CodeRabbit Chat: Implement requested code changes#126coderabbitai[bot] wants to merge 4 commits into
Conversation
Every external command was logged twice: once at INFO by subprocess_runner via log_command_invocation, and again at DEBUG by invoke_via_subprocess via _log_subprocess_spawn, with two different renderings of the same command line. Keep the INFO-level record (it is the operationally visible one and already includes the working directory) and delete the redundant DEBUG spawn line. The DEBUG record for environment overrides is retained because it carries information the INFO line does not. Add regression tests pinning that a command produces exactly one invocation log record, at INFO, with and without a cwd. Closes #104
Address review feedback on the single-invocation-log regression test:
- Loosen the INFO message assertion so it checks the stable prefix
("Running external command") and the command ("echo hello") rather
than the full formatted string, keeping the test robust to wording
or spacing changes.
- Restrict `_invocation_records` to the INFO command line only, and add
an explicit `_assert_no_spawn_record` check that the removed DEBUG
"Spawning subprocess:" log is absent, tying the regression directly
to #104.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The cmd-mox passthrough path in `_handle_cmd_mox_passthrough` calls `invoke_via_subprocess` directly rather than going through `subprocess_runner`, so it never reached the `log_command_invocation` call. Once the redundant DEBUG spawn log was removed for #104, these passthrough external commands emitted no command/cwd invocation record at all, regressing command observability. Emit the single INFO invocation record at the passthrough call site via `log_command_invocation`, mirroring `subprocess_runner`. Routing through `subprocess_runner` is not viable here because that wrapper does not accept `stdin_data`, which the passthrough path needs. Extend the passthrough streaming test to assert exactly one INFO invocation record is logged for the passthrough command. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Important Review skippedThis PR was authored by the user configured for CodeRabbit reviews. CodeRabbit does not review PRs authored by this user. It's recommended to use a dedicated user account to post CodeRabbit review feedback. ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
Reviewer's guide (collapsed on small PRs)Reviewer's GuideAdjusts the test for cmd_mox passthrough logging to obtain fixtures dynamically via pytest’s request object and extends typing helpers to include FixtureRequest for both runtime and typing-only branches. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- Consider keeping
caploganduse_real_invokeas explicit test parameters instead of fetching them viarequest.getfixturevalue, unless you truly need the dynamic lookup, as explicit fixture injection is clearer and better communicates test dependencies.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider keeping `caplog` and `use_real_invoke` as explicit test parameters instead of fetching them via `request.getfixturevalue`, unless you truly need the dynamic lookup, as explicit fixture injection is clearer and better communicates test dependencies.
## Individual Comments
### Comment 1
<location path="tests/unit/publish/test_command_logging.py" line_range="94" />
<code_context>
caplog.set_level(logging.INFO, logger="lading.testing.cmd_mox_runner")
monkeypatch.setenv("LADING_USE_CMD_MOX_STUB", "1")
script = "print('unused')"
- cmd_mox.spy(sys.executable).with_args("-c", script).passthrough()
+
</code_context>
<issue_to_address>
**issue (testing):** The test no longer invokes `cmd_mox`, so it may not validate passthrough behavior anymore.
With the removal of `cmd_mox.spy(sys.executable).with_args("-c", script).passthrough()` and no replacement, `test_cmd_mox_passthrough_streams_output` no longer appears to trigger the passthrough/streaming behavior it’s supposed to verify. This risks making the test effectively a no-op. Please reintroduce an explicit `cmd_mox` invocation (adapted if needed) or add a new call that actually exercises passthrough so the test still confirms that subprocess output is streamed and logged.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| caplog.set_level(logging.INFO, logger="lading.testing.cmd_mox_runner") | ||
| monkeypatch.setenv("LADING_USE_CMD_MOX_STUB", "1") | ||
| script = "print('unused')" | ||
| cmd_mox.spy(sys.executable).with_args("-c", script).passthrough() |
There was a problem hiding this comment.
issue (testing): The test no longer invokes cmd_mox, so it may not validate passthrough behavior anymore.
With the removal of cmd_mox.spy(sys.executable).with_args("-c", script).passthrough() and no replacement, test_cmd_mox_passthrough_streams_output no longer appears to trigger the passthrough/streaming behavior it’s supposed to verify. This risks making the test effectively a no-op. Please reintroduce an explicit cmd_mox invocation (adapted if needed) or add a new call that actually exercises passthrough so the test still confirms that subprocess output is streamed and logged.
Code changes was requested by @leynos.
The following files were modified:
tests/unit/publish/test_command_logging.pySummary by Sourcery
Tests: