Log external command invocations - #26
Conversation
Reviewer's GuideThe PR introduces a shared helper for formatting and logging external command invocations, applies it to both the workspace metadata loader and the publish command, refactors cargo metadata command composition into constants, and adds unit tests to verify the new logging behavior. Sequence diagram for logging external command invocation in publish commandsequenceDiagram
participant LOGGER
participant Publish
participant log_command_invocation
participant ExternalCommand
Publish->>log_command_invocation: log_command_invocation(LOGGER, command, cwd)
log_command_invocation->>LOGGER: info("Running external command: ...")
Publish->>ExternalCommand: execute(command, cwd)
Sequence diagram for logging external command invocation in workspace metadata loadingsequenceDiagram
participant LOGGER
participant WorkspaceMetadata
participant log_command_invocation
participant Cargo
WorkspaceMetadata->>log_command_invocation: log_command_invocation(LOGGER, command, root_path)
log_command_invocation->>LOGGER: info("Running external command: ...")
WorkspaceMetadata->>Cargo: run(command, cwd=root_path)
Class diagram for new process helpers and logging integrationclassDiagram
class log_command_invocation {
+log_command_invocation(logger, command, cwd)
}
class format_command {
+format_command(command)
}
class workspace_metadata {
+load_cargo_metadata(workspace_root)
}
class commands_publish {
+_invoke(command, cwd)
}
log_command_invocation <|-- format_command
workspace_metadata ..> log_command_invocation : uses
commands_publish ..> log_command_invocation : uses
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdded a small command-formatting/logging utility and wired it into publish command and cargo-metadata loading so each external command invocation is logged (with optional cwd). New unit tests verify formatting and logged messages. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Publish as publish._invoke
participant Logger as log_command_invocation
participant Subproc as subprocess (run/invoke)
Note over Publish,Logger `#f8f4e6`: New logging hook before execution
Publish->>Logger: format & log command (+cwd)
Logger-->>Publish: OK
Publish->>Subproc: execute command
Subproc-->>Publish: stdout/stderr/exit
sequenceDiagram
autonumber
participant Loader as load_cargo_metadata
participant Logger as log_command_invocation
participant CmdEns as _ensure_command / _CmdMoxCommand
participant Subproc as subprocess / cmd-mox IPC
Note over Loader,Logger `#eef7ff`: Use _CARGO_PROGRAM and _CARGO_METADATA_ARGS
Loader->>CmdEns: determine command (program + args)
CmdEns->>Loader: command tuple / argv
Loader->>Logger: format & log cargo metadata invocation (+cwd)
Logger-->>Loader: OK
Loader->>Subproc: invoke cargo (direct or IPC)
Subproc-->>Loader: metadata JSON / exit
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used📓 Path-based instructions (3)**/*.py📄 CodeRabbit inference engine (.rules/python-00.md)
Files:
{**/unittests/test_*.py,tests/**/*.py}📄 CodeRabbit inference engine (.rules/python-00.md)
Files:
tests/**/*.py📄 CodeRabbit inference engine (.rules/python-exception-design-raising-handling-and-logging.md)
Files:
🧬 Code graph analysis (3)lading/workspace/metadata.py (2)
tests/unit/publish/test_command_logging.py (1)
tests/unit/utils/test_process.py (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
🔇 Additional comments (6)
Comment |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes and found some issues that need to be addressed.
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `lading/workspace/metadata.py:108-111` </location>
<code_context>
"""Execute ``cargo metadata`` and parse the resulting JSON payload."""
command = _ensure_command()
root_path = normalise_workspace_root(workspace_root)
+ log_command_invocation(LOGGER, _CARGO_METADATA_COMMAND, root_path)
exit_code, stdout, stderr = command.run(retcode=None, cwd=str(root_path))
stdout_text = _coerce_text(stdout)
</code_context>
<issue_to_address>
**suggestion:** Consider logging the actual command that will be executed, including resolved arguments.
If command arguments become dynamic in the future, ensure the log reflects the actual command object used for execution.
```suggestion
command = _ensure_command()
root_path = normalise_workspace_root(workspace_root)
# Log the actual command object, including resolved arguments
log_command_invocation(LOGGER, str(command), root_path)
exit_code, stdout, stderr = command.run(retcode=None, cwd=str(root_path))
```
</issue_to_address>
### Comment 2
<location> `lading/workspace/metadata.py:94` </location>
<code_context>
except CommandNotFound as exc:
raise CargoExecutableNotFoundError from exc
- return cargo["metadata", "--format-version", "1"]
+ return cargo[list(_CARGO_METADATA_ARGS)]
</code_context>
<issue_to_address>
**suggestion:** Passing a list to cargo[...] may be unnecessary if _CARGO_METADATA_ARGS is already a tuple.
Consider passing _CARGO_METADATA_ARGS directly to cargo[...] to avoid unnecessary conversion and improve code clarity.
```suggestion
return cargo[_CARGO_METADATA_ARGS]
```
</issue_to_address>
### Comment 3
<location> `lading/utils/process.py:16-18` </location>
<code_context>
+ PathType = typ.Any
+
+
+def _command_as_tuple(command: typ.Sequence[str]) -> tuple[str, ...]:
+ """Return ``command`` as an immutable tuple of strings."""
+ return tuple(command)
+
+
</code_context>
<issue_to_address>
**suggestion:** The _command_as_tuple helper may be redundant.
You can remove _command_as_tuple and pass command directly to shlex.join, as it already accepts any sequence.
Suggested implementation:
```python
```
If `_command_as_tuple` is used elsewhere in the file, replace those usages by passing the `command` sequence directly to `shlex.join` or other functions that accept a sequence. For example, change `shlex.join(_command_as_tuple(command))` to `shlex.join(command)`.
</issue_to_address>
### Comment 4
<location> `lading/utils/process.py:21-26` </location>
<code_context>
+def format_command(command: typ.Sequence[str]) -> str:
+ """Return a shell-style representation of ``command`` for logging."""
+ command_tuple = _command_as_tuple(command)
+ if not command_tuple:
+ return ""
+ return shlex.join(command_tuple)
+
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Returning an empty string for an empty command may obscure logging intent.
Consider logging a warning or raising an exception when an empty command is received, as this likely indicates a mistake and may hinder debugging.
```suggestion
import logging
def format_command(command: typ.Sequence[str]) -> str:
"""Return a shell-style representation of ``command`` for logging."""
command_tuple = _command_as_tuple(command)
if not command_tuple:
logging.warning("format_command received an empty command sequence. This may indicate a bug or misconfiguration.")
return ""
return shlex.join(command_tuple)
```
</issue_to_address>
### Comment 5
<location> `tests/unit/publish/test_command_logging.py:21-30` </location>
<code_context>
+ LogCaptureFixture = typ.Any
+
+
+def test_invoke_logs_command_with_cwd(
+ tmp_path: Path, caplog: LogCaptureFixture
+) -> None:
+ """``_invoke`` should log the command line and working directory."""
+ caplog.set_level(logging.INFO, logger="lading.commands.publish")
+ exit_code, stdout, stderr = publish._invoke(("echo", "hello"), cwd=tmp_path)
+
+ assert exit_code == 0
+ assert stdout.strip() == "hello"
+ assert stderr == ""
+ expected = f"Running external command: echo hello (cwd={tmp_path})"
+ assert expected in caplog.messages
</code_context>
<issue_to_address>
**suggestion (testing):** Consider adding a test for _invoke when cwd is None.
Please add a test verifying that when cwd is None, the log message does not include the (cwd=...) portion.
</issue_to_address>
### Comment 6
<location> `lading/utils/process.py:29` </location>
<code_context>
+ return shlex.join(command_tuple)
+
+
+def log_command_invocation(
+ logger: LoggerType,
+ command: typ.Sequence[str],
</code_context>
<issue_to_address>
**issue (review_instructions):** Add behavioural and unit tests for log_command_invocation and format_command.
You have added new process execution helpers, but there are no corresponding behavioural or unit tests verifying their functionality. Add tests to ensure correct logging and command formatting.
<details>
<summary>Review instructions:</summary>
**Path patterns:** `**/*`
**Instructions:**
For any new feature or change to an existing feature, both behavioural *and* unit tests are required.
</details>
</issue_to_address>
### Comment 7
<location> `lading/utils/process.py:24-26` </location>
<code_context>
def format_command(command: typ.Sequence[str]) -> str:
"""Return a shell-style representation of ``command`` for logging."""
command_tuple = _command_as_tuple(command)
if not command_tuple:
return ""
return shlex.join(command_tuple)
</code_context>
<issue_to_address>
**suggestion (code-quality):** We've found these issues:
- Lift code into else after jump in control flow ([`reintroduce-else`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/reintroduce-else/))
- Replace if statement with if expression ([`assign-if-exp`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/assign-if-exp/))
```suggestion
return "" if not command_tuple else shlex.join(command_tuple)
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![codescene-delta-analysis[bot]](https://avatars.githubusercontent.com/in/53921?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
lading/utils/process.py (1)
16-18: Consider inlining this helper.The
_command_as_tuplefunction is only called once informat_commandand simply wrapstuple(command). Inlining this conversion directly informat_commandwould reduce indirection without sacrificing clarity.Apply this diff if you'd like to simplify:
-def _command_as_tuple(command: typ.Sequence[str]) -> tuple[str, ...]: - """Return ``command`` as an immutable tuple of strings.""" - return tuple(command) - - def format_command(command: typ.Sequence[str]) -> str: """Return a shell-style representation of ``command`` for logging.""" - command_tuple = _command_as_tuple(command) + command_tuple = tuple(command) if not command_tuple: return "" return shlex.join(command_tuple)
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
lading/commands/publish.py(4 hunks)lading/utils/process.py(1 hunks)lading/workspace/metadata.py(5 hunks)tests/unit/publish/test_command_logging.py(1 hunks)tests/unit/test_workspace_metadata.py(2 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.py
📄 CodeRabbit inference engine (.rules/python-00.md)
**/*.py: Python files must use snake_case filenames (e.g., http_client.py, task_queue.py)
Classes must use PascalCase
Variables and functions must use snake_case
Module-level constants must use UPPER_SNAKE_CASE
Prefix non-exported helpers or internal APIs with a single leading underscore
Use typing everywhere and maintain full static type coverage
Use TypedDict or @DataClass for structured data; prefer @DataClass(slots=True) for internal-only usage
Avoid Any; prefer precise types (TypeVar, Protocol, Literal, Union); use typing.cast only when necessary with justification; use object for unknown opaque values
Be explicit with return types (e.g., -> None, -> str) for all public functions and methods
Favor immutability: prefer tuples to lists and MappingProxyType for read-only mappings; document third-party frozendict if used
Use# pyright: ignoresparingly and include an explanation comment when used
Avoid side effects at import time (no global state mutation or actions on import)
Use docstrings (NumPy format) for public functions, classes, and modules
Explain tricky code with inline comments
**/*.py: Prefer context managers to encapsulate setup/teardown for resources (files, locks, connections) instead of manual try/finally blocks
Use contextlib.contextmanager (@contextmanager) to implement simple, linear setup/teardown context managers
Implement a class with enter and exit for context managers that require internal state or more complex lifecycle handling
For file I/O, prefer with open(...) as ... over open()/try/finally/close patterns
Choose @contextmanager when control flow is linear and no persistent state is needed
Choose a class-based context manager when there is internal state, lifecycle methods, re-entry, or advanced context needs
**/*.py: Exception classes must end with the suffix 'Error' (N818)
Prefer specific built-ins (e.g., TypeError, ValueError) or domain exceptions over raising Exception directly (TRY003/TRY004)
Preserve causal chains...
Files:
lading/workspace/metadata.pylading/commands/publish.pytests/unit/test_workspace_metadata.pytests/unit/publish/test_command_logging.pylading/utils/process.py
{**/unittests/test_*.py,tests/**/*.py}
📄 CodeRabbit inference engine (.rules/python-00.md)
{**/unittests/test_*.py,tests/**/*.py}: Use pytest idioms: prefer fixtures over setup/teardown, parametrize broadly, avoid unnecessary mocks
Group related tests using classes with method names prefixed by test_
Write tests from a user's perspective: test public behaviour, not internals
Files:
tests/unit/test_workspace_metadata.pytests/unit/publish/test_command_logging.py
tests/**/*.py
📄 CodeRabbit inference engine (.rules/python-exception-design-raising-handling-and-logging.md)
In tests, assert specific exception types (and optional message via regex) rather than using broad Exception (B017)
Files:
tests/unit/test_workspace_metadata.pytests/unit/publish/test_command_logging.py
🧬 Code graph analysis (4)
lading/workspace/metadata.py (2)
lading/utils/path.py (1)
normalise_workspace_root(10-16)lading/utils/process.py (1)
log_command_invocation(29-39)
lading/commands/publish.py (1)
lading/utils/process.py (1)
log_command_invocation(29-39)
tests/unit/test_workspace_metadata.py (1)
lading/workspace/metadata.py (2)
run(130-152)load_cargo_metadata(104-122)
tests/unit/publish/test_command_logging.py (1)
lading/commands/publish.py (1)
_invoke(678-702)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Sourcery review
🔇 Additional comments (15)
lading/commands/publish.py (3)
7-7: LGTM: Imports added for logging functionality.The logging import and the
log_command_invocationutility import are correctly placed and support the new logging instrumentation.Also applies to: 16-16
27-28: LGTM: Module logger follows best practices.The module-level logger is correctly initialized using
logging.getLogger(__name__), following the coding guidelines for avoiding the root logger.
682-682: LGTM: Command invocation logging correctly placed.The logging call is appropriately positioned at the start of
_invoke, ensuring all external command executions are traced before they run.tests/unit/publish/test_command_logging.py (1)
21-32: LGTM: Comprehensive test for command logging.The test thoroughly verifies that
_invokelogs the command line and working directory, while also confirming the command executes correctly. The use ofcaplogand specific logger targeting is appropriate.tests/unit/test_workspace_metadata.py (2)
7-7: LGTM: Logging import added for test.The logging import supports the new test that verifies command logging behavior.
322-353: LGTM: Well-structured test for cargo metadata logging.The test effectively verifies that
load_cargo_metadatalogs the cargo command with its working directory. The fake command approach isolates the logging behavior from actual cargo execution, and the assertion correctly accounts for path resolution.lading/workspace/metadata.py (5)
6-6: LGTM: Logging imports correctly added.The logging infrastructure imports are properly placed to support the new command logging functionality.
Also applies to: 14-14
78-83: LGTM: Well-structured cargo command constants.The introduction of
_CARGO_PROGRAM,_CARGO_METADATA_ARGS, and_CARGO_METADATA_COMMANDcentralizes the cargo invocation details, making the code more maintainable. The module logger follows best practices.
91-91: LGTM: Constants correctly applied in command construction.The use of
_CARGO_PROGRAMand_CARGO_METADATA_ARGSconstants eliminates hardcoded strings and ensures consistency across the module.Also applies to: 94-94
110-110: LGTM: Cargo metadata logging correctly integrated.The logging call is properly positioned to trace cargo metadata invocations before execution, using the normalized workspace root path for context.
128-128: LGTM: Constants consistently applied in cmd-mox path.The cmd-mox command proxy correctly uses
_CARGO_METADATA_ARGSand_CARGO_PROGRAM, ensuring consistent cargo invocation across both direct and IPC execution paths.Also applies to: 146-146
lading/utils/process.py (4)
1-14: LGTM: Well-structured module header and type imports.The module follows best practices with
from __future__ import annotationsand usesTYPE_CHECKINGto avoid runtime import costs for type hints.
21-26: LGTM: Command formatting uses shell-safe representation.The use of
shlex.joinensures proper shell quoting and escaping for logged commands. The empty command guard is a good defensive practice.
29-39: LGTM: Logging function follows parameterized logging best practices.The implementation correctly uses parameterized logging with
%splaceholders rather than f-strings, adhering to the coding guidelines (LOG004). The conditional formatting forcwdis clear and appropriate.
42-42: LGTM: Public API correctly defined.The
__all__export list appropriately includes only the public functions, keeping the private helper internal.
There was a problem hiding this comment.
Gates Passed
See analysis details in CodeScene
Absence of Expected Change Pattern
- lading/lading/commands/publish.py is usually changed with: lading/tests/bdd/steps/test_publish_steps.py
Quality Gate Profile: Pay Down Tech Debt
Want more control? Customize Code Health rules or catch issues early with our IDE extension and CLI tool.
1 participant
Summary
Testing
https://chatgpt.com/codex/tasks/task_e_690a120bf3788322b94f4321ebcf176d
Summary by Sourcery
Log external commands before execution by introducing a shared helper and applying it to cargo metadata loading and publish operations.
New Features:
Enhancements:
Tests:
Summary by CodeRabbit
Chores
Tests