Move cmd-mox behind a CommandRunner port (#62) - #76
Conversation
Add a runtime `CommandRunner` port and subprocess adapter so production commands depend on a stable command-execution abstraction. Move cmd-mox IPC and passthrough handling into `lading.testing`, then select that adapter once at the CLI boundary when `LADING_USE_CMD_MOX_STUB` is enabled. Route cargo metadata and publish preflight execution through the selected runner so production modules no longer import the dev-only dependency.
|
Warning Review limit reached
More reviews will be available in 55 minutes and 50 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (24)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix. Include the file and symbol names indicated in the issue at the head of your response. Comment on lines +114 to +115 *,
runner: CommandRunner | None = None,❌ New issue: Complex Method |
This comment was marked as resolved.
This comment was marked as resolved.
Move command invocation and JSON parsing out of `load_cargo_metadata` into private helpers. Keep the public API unchanged while reducing the function's cyclomatic complexity.
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
Make command invocation logging and stdout echo policy owned by the runtime command runner. Move text coercion into `lading.runtime`, clean up publish shim aliases, and point helper tests at the modules that own the code.
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix. Include the file and symbol names indicated in the issue at the head of your response. lading/runtime/subprocess_runner.py Comment on lines +125 to +204 def invoke_via_subprocess(
program: str,
args: tuple[str, ...],
context: SubprocessContext,
) -> tuple[int, str, str]:
"""Spawn ``program`` with ``args`` while proxying its output streams.
Parameters
----------
program:
Program to execute.
args:
Arguments to pass to ``program``.
context:
Working directory, environment, and optional stdin payload.
Returns
-------
tuple[int, str, str]
Exit code, captured stdout, and captured stderr.
Raises
------
CommandSpawnError
If the subprocess cannot be created.
"""
command = (program, *args)
_log_subprocess_spawn(command, context.cwd)
_log_subprocess_environment(context.env)
normalised_env = normalise_environment(context.env)
try:
# This path owns `Popen` directly so relay threads can drain both pipes
# before the function returns. S603 is mitigated because `command` is a
# pre-split sequence and the default `shell=False` is used.
process = subprocess.Popen( # noqa: S603 # pylint: disable=consider-using-with
command,
cwd=None if context.cwd is None else str(context.cwd),
env=normalised_env,
stdout=subprocess.PIPE,
stderr=subprocess.PIPE,
stdin=subprocess.PIPE if context.stdin_data is not None else None,
)
except OSError as exc:
raise CommandSpawnError(program, exc) from exc
stdout_chunks: list[str] = []
stderr_chunks: list[str] = []
threads = [
threading.Thread(
target=relay_stream,
args=(
process.stdout,
sys.stdout if context.echo_stdout else None,
stdout_chunks,
),
name=_format_thread_name(program, "stdout"),
daemon=True,
),
threading.Thread(
target=relay_stream,
args=(process.stderr, sys.stderr, stderr_chunks),
name=_format_thread_name(program, "stderr"),
daemon=True,
),
]
for thread in threads:
thread.start()
try:
if context.stdin_data is not None and process.stdin is not None:
try:
process.stdin.write(context.stdin_data.encode("utf-8"))
process.stdin.close()
except BrokenPipeError:
_LOGGER.debug("Process closed stdin before all data was written")
finally:
exit_code = process.wait()
for thread in threads:
thread.join()
return exit_code, "".join(stdout_chunks), "".join(stderr_chunks)❌ New issue: Large Method |
This comment was marked as resolved.
This comment was marked as resolved.
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix. Include the file and symbol names indicated in the issue at the head of your response. lading/runtime/subprocess_runner.py Comment on lines +125 to +204 def invoke_via_subprocess(
program: str,
args: tuple[str, ...],
context: SubprocessContext,
) -> tuple[int, str, str]:
"""Spawn ``program`` with ``args`` while proxying its output streams.
Parameters
----------
program:
Program to execute.
args:
Arguments to pass to ``program``.
context:
Working directory, environment, and optional stdin payload.
Returns
-------
tuple[int, str, str]
Exit code, captured stdout, and captured stderr.
Raises
------
CommandSpawnError
If the subprocess cannot be created.
"""
command = (program, *args)
_log_subprocess_spawn(command, context.cwd)
_log_subprocess_environment(context.env)
normalised_env = normalise_environment(context.env)
try:
# This path owns `Popen` directly so relay threads can drain both pipes
# before the function returns. S603 is mitigated because `command` is a
# pre-split sequence and the default `shell=False` is used.
process = subprocess.Popen( # noqa: S603 # pylint: disable=consider-using-with
command,
cwd=None if context.cwd is None else str(context.cwd),
env=normalised_env,
stdout=subprocess.PIPE,
stderr=subprocess.PIPE,
stdin=subprocess.PIPE if context.stdin_data is not None else None,
)
except OSError as exc:
raise CommandSpawnError(program, exc) from exc
stdout_chunks: list[str] = []
stderr_chunks: list[str] = []
threads = [
threading.Thread(
target=relay_stream,
args=(
process.stdout,
sys.stdout if context.echo_stdout else None,
stdout_chunks,
),
name=_format_thread_name(program, "stdout"),
daemon=True,
),
threading.Thread(
target=relay_stream,
args=(process.stderr, sys.stderr, stderr_chunks),
name=_format_thread_name(program, "stderr"),
daemon=True,
),
]
for thread in threads:
thread.start()
try:
if context.stdin_data is not None and process.stdin is not None:
try:
process.stdin.write(context.stdin_data.encode("utf-8"))
process.stdin.close()
except BrokenPipeError:
_LOGGER.debug("Process closed stdin before all data was written")
finally:
exit_code = process.wait()
for thread in threads:
thread.join()
return exit_code, "".join(stdout_chunks), "".join(stderr_chunks)❌ New issue: Large Method |
This comment was marked as resolved.
This comment was marked as resolved.
Split process spawning and stdin draining out of `invoke_via_subprocess` so the runner stays below the complexity and line-count threshold without changing command execution behaviour.
There was a problem hiding this comment.
Our agent can fix these. Install it.
Gates Passed
6 Quality Gates Passed
Absence of Expected Change Pattern
- lading/lading/cli.py is usually changed with: lading/tests/unit/test_cli.py
- lading/lading/commands/publish.py is usually changed with: lading/tests/bdd/steps/test_publish_steps.py
Quality Gate Profile: Pay Down Tech Debt
Install CodeScene MCP: safeguard and uplift AI-generated code. Catch issues early with our IDE extension and CLI tool.
|
@coderabbitai review |
✅ Action performedReview finished.
|
`publish_plan` defined `_append_section` and `_format_plan` privately and then re-bound them to public aliases; every consumer used the public names. Rename the definitions to `append_section` and `format_plan`, drop the aliases, and remove the underscore variants (plus the internal `_format_crates_section`) from `__all__`. `publish_execution` carried a `split_command` wrapper over `lading.runtime.subprocess_runner.split_command` whose only purpose was mapping `ValueError` to `PublishPreflightError`. Nothing in production code called it — `cmd_mox_runner` uses the runtime helper directly — so remove the wrapper, its alias, and its only test. Refresh the publish-flow diagram in `docs/lading-design.md`, which still showed `split_command` and the cmd-mox helpers under `publish_execution`; those moved to `lading.testing.cmd_mox_runner` in #76.
`publish_plan` defined `_append_section` and `_format_plan` privately and then re-bound them to public aliases; every consumer used the public names. Rename the definitions to `append_section` and `format_plan`, drop the aliases, and remove the underscore variants (plus the internal `_format_crates_section`) from `__all__`. `publish_execution` carried a `split_command` wrapper over `lading.runtime.subprocess_runner.split_command` whose only purpose was mapping `ValueError` to `PublishPreflightError`. Nothing in production code called it — `cmd_mox_runner` uses the runtime helper directly — so remove the wrapper, its alias, and its only test. Refresh the publish-flow diagram in `docs/lading-design.md`, which still showed `split_command` and the cmd-mox helpers under `publish_execution`; those moved to `lading.testing.cmd_mox_runner` in #76.
`publish_plan` defined `_append_section` and `_format_plan` privately and then re-bound them to public aliases; every consumer used the public names. Rename the definitions to `append_section` and `format_plan`, drop the aliases, and remove the underscore variants (plus the internal `_format_crates_section`) from `__all__`. `publish_execution` carried a `split_command` wrapper over `lading.runtime.subprocess_runner.split_command` whose only purpose was mapping `ValueError` to `PublishPreflightError`. Nothing in production code called it — `cmd_mox_runner` uses the runtime helper directly — so remove the wrapper, its alias, and its only test. Refresh the publish-flow diagram in `docs/lading-design.md`, which still showed `split_command` and the cmd-mox helpers under `publish_execution`; those moved to `lading.testing.cmd_mox_runner` in #76.
* Promote canonical names in publish_plan and publish_execution (#163) `publish_plan` defined `_append_section` and `_format_plan` privately and then re-bound them to public aliases; every consumer used the public names. Rename the definitions to `append_section` and `format_plan`, drop the aliases, and remove the underscore variants (plus the internal `_format_crates_section`) from `__all__`. `publish_execution` carried a `split_command` wrapper over `lading.runtime.subprocess_runner.split_command` whose only purpose was mapping `ValueError` to `PublishPreflightError`. Nothing in production code called it — `cmd_mox_runner` uses the runtime helper directly — so remove the wrapper, its alias, and its only test. Refresh the publish-flow diagram in `docs/lading-design.md`, which still showed `split_command` and the cmd-mox helpers under `publish_execution`; those moved to `lading.testing.cmd_mox_runner` in #76. * Remove preflight compatibility shims from publish (#163) Delete the backwards-compatibility layer that `publish.py` kept for test patches after the pre-flight helpers moved to `publish_preflight` (#96, #123): - Drop the eleven bare aliases (`_preflight_argument_sets`, `_CargoPreflightOptions`, `_run_cargo_preflight`, and friends). - Drop the thin `_run_preflight_checks` wrapper that resolved an optional `configuration`; `run()` already resolves configuration before the call, so it now invokes `publish_preflight._run_preflight_checks` directly. - Drop the unused `_append_section`, `_format_plan`, `metadata_module`, `StripPatchesSetting`, and `PublishPlanError` re-bindings. Private symbols carry no stability contract and lading is the only consumer of its own internals, so tests now patch and call the canonical module: conftest fixtures stub `publish_preflight._run_preflight_checks`, the preflight suites target `publish_preflight` directly, and the plan-validation tests raise `publish_plan.PublishPlanError`. The two tests that pinned the wrapper's aliasing behaviour are removed with the wrapper. * Remove bump_toml compatibility aliases from bump (#163) Drop the six module-level re-exports (`_parse_manifest`, `_select_table`, `_assign_version`, `_value_matches`, `_update_dependency_sections`, `_update_dependency_table`) that `bump.py` kept solely so existing tests could keep patching the old names. The TOML helper tests now exercise `bump_toml` directly. Also fold the redundant `_log = LOGGER` alias into a single `LOGGER` name; the README-transposition failure path now logs via `LOGGER.exception`, matching the logging convention enforced by lint for handlers that re-raise. * Document the no-compatibility-alias convention (#163) Record in the developers' guide that private symbols carry no stability contract: when a helper moves to a new canonical module, update call sites and test patch targets in the same change instead of leaving aliases or thin wrappers behind. Point recurring churn at an explicit port (as with `CommandRunner`) rather than ad hoc shims. Rewrite the pre-flight validation section, which still described the `publish.py` re-exports and optional-configuration wrapper removed in this branch. * Restore public PublishPlanError re-export on publish (#163) The shim sweep removed `PublishPlanError` from `publish.py` alongside the private compatibility aliases. That exception is public API, however: the still-exported `publish.plan_publication()` raises it, so callers catching `except publish.PublishPlanError` began failing with `AttributeError`. Re-export it explicitly with `PublishPlanError as PublishPlanError` so the public entry point and the exception it raises stay catchable from the same module. The no-alias convention in the developers' guide is clarified to scope it to private, underscore-prefixed symbols, and a regression test asserts `publish.PublishPlanError` remains the canonical class and catches planning failures. * Document shim inventory and public plan-helper APIs (#163) Address review feedback on the compatibility-shim sweep. Expand the "Internal APIs carry no compatibility aliases" section of the developers' guide with a repository-wide inventory mapping every removed private alias, wrapper, and re-export to its canonical replacement, plus a "Retained boundaries" subsection recording why `publish._invoke`, the public `publish.PublishPlanError` re-export, and the `CommandRunner` port are kept. This gives issue #163 an explicit audit trail. Give the now-public `append_section` and `format_plan` helpers full NumPy-style `Parameters`/`Returns` docstrings, and tighten `test_publish_reexports_plan_error_for_public_callers` with a diagnostic assertion message and a `match="dependency cycle"` constraint on the raised `PublishPlanError`. * Refresh typos.toml from the Oxford-spelling authority The markdownlint gate runs `scripts/typos_rollout.py`, which regenerates `typos.toml` from its upstream authority. The tracked file had drifted behind the authority, so the gate refreshed it, adding the `polymerize` word family. Commit the deterministic regeneration to keep the working tree clean; the content is generated, not hand-authored, and unrelated to the shim sweep. * Add doctest examples to publish plan helpers (#163) Address review feedback asking the public `append_section` and `format_plan` helpers to demonstrate representative usage. Add NumPy-style `Examples` sections with executable `>>>` doctests, matching the house style used by helpers such as `command_detail`. The `append_section` example shows both the append and the empty-items no-op; the `format_plan` example renders an empty plan. Both doctests pass under `python -m doctest`. Existing `Parameters` and `Returns` sections are unchanged. * Caption the shim inventory and make the plan doctest portable (#163) Address two review nits on the compatibility-shim documentation. Add a `#### Shim inventory` subheading before the inventory table in the developers' guide so the table is captioned, consistent with the sibling `#### Retained boundaries` heading. A heading is used rather than bold text because markdownlint MD036 (emphasis-as-heading) is active. Construct the `format_plan` doctest with a relative `Path("ws")` instead of an absolute `Path("/ws")` so the rendered "Publish plan for ws" line is platform-neutral (an absolute path renders differently on Windows). The doctest still passes. --------- Co-authored-by: leynos <leynos@rohga>
Summary
This branch moves the cmd-mox IPC integration for issue #62 out of production modules and behind a
CommandRunnerport selected at the CLI boundary.Closes #62.
Production code now depends on the runtime command-runner protocol and default subprocess adapter, while the cmd-mox adapter lives under
lading.testingand is loaded only whenLADING_USE_CMD_MOX_STUBis enabled.Review walkthrough
Validation
make check-fmt: passed.make lint: passed with Ruff clean and Pylint at 10.00/10.make typecheck: passed withty checkclean.make test: passed, 465 tests and 14 snapshots.grep -R "cmd_mox" -n lading: passed with hits only under lading/testing/cmd_mox_runner.py.coderabbit review --agent: passed with zero findings after follow-up fixes.