Add Cyclopts CLI scaffolding for lading - #5
Conversation
Introduce the lading package with a Cyclopts-powered CLI shell.
Reviewer's GuideThis PR adds initial scaffolding for the lading CLI using Cyclopts, including placeholder bump and publish commands, comprehensive tests and behaviour suites, updated documentation, and necessary project configuration changes to integrate the new package. Sequence diagram for lading CLI invocation and workspace root handlingsequenceDiagram
actor User
participant CLI as lading.cli.main
participant Env as Environment
participant Cyclopts as cyclopts.App
User->>CLI: Run `python -m lading.cli [--workspace-root <path>] <subcommand>`
CLI->>CLI: _extract_workspace_override(argv)
CLI->>CLI: _normalise_workspace_root(workspace_override)
CLI->>Env: Set LADING_WORKSPACE_ROOT env var
CLI->>Cyclopts: Dispatch subcommand (bump/publish)
Cyclopts->>CLI: Return result
CLI->>User: Print acknowledgement message
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Warning Rate limit exceeded@leynos has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 2 minutes and 45 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, 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 have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (2)
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. WalkthroughAdds a Cyclopts-based CLI scaffold for lading (app, main, bump, publish) with workspace-root parsing and temporary LADING_WORKSPACE_ROOT handling, package and typecheck config updates, filesystem helper and exports, documentation and BDD/unit tests, and a Makefile typecheck tweak to include Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant CLI as lading.cli.main
participant Env as LADING_WORKSPACE_ROOT
participant App as Cyclopts App
participant Cmd as command.run
participant Out as Stdout
User->>CLI: python -m lading.cli [--workspace-root <p>] <subcmd>
CLI->>CLI: extract last --workspace-root, validate, normalise
alt missing or malformed
CLI-->>User: SystemExit (workspace-root required)
else valid
CLI->>Env: set LADING_WORKSPACE_ROOT (temporarily)
CLI->>App: dispatch remaining argv
App->>Cmd: run(workspace_root)
Cmd-->>App: "placeholder invoked for <path>"
App-->>CLI: result string
CLI->>Out: print(result)
CLI->>Env: restore previous LADING_WORKSPACE_ROOT
CLI-->>User: exit 0
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
Comment |
![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.
![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 - here's some feedback:
- Consider adding a console_scripts entry point in pyproject.toml so users can invoke
ladingdirectly instead ofpython -m lading.cli. - The usage guide links to
cmd-mox-usage-guide.mdbut that file isn’t in the diff—either add it or remove/update the broken link. - The workspace‐root normalization logic is duplicated in both
cli.pyand the command modules; extracting it into a shared helper could reduce redundancy.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider adding a console_scripts entry point in pyproject.toml so users can invoke `lading` directly instead of `python -m lading.cli`.
- The usage guide links to `cmd-mox-usage-guide.md` but that file isn’t in the diff—either add it or remove/update the broken link.
- The workspace‐root normalization logic is duplicated in both `cli.py` and the command modules; extracting it into a shared helper could reduce redundancy.
## Individual Comments
### Comment 1
<location> `lading/cli.py:96-103` </location>
<code_context>
+ return 0
+
+
+def main(argv: typ.Sequence[str] | None = None) -> int:
+ """Entry point for ``python -m lading.cli``."""
+ if argv is None:
</code_context>
<issue_to_address>
**suggestion:** main does not handle KeyboardInterrupt or unexpected exceptions.
Catching these exceptions will allow the CLI to exit gracefully and display a user-friendly error message.
```suggestion
def main(argv: typ.Sequence[str] | None = None) -> int:
"""Entry point for ``python -m lading.cli``."""
try:
if argv is None:
argv = sys.argv[1:]
workspace_override, remaining = _extract_workspace_override(list(argv))
workspace_root = _normalise_workspace_root(workspace_override)
with _workspace_env(workspace_root):
return _dispatch_and_print(remaining)
except KeyboardInterrupt:
print("\nOperation cancelled by user.", file=sys.stderr)
return 130 # Standard exit code for interrupted process
except Exception as exc:
print(f"Unexpected error: {exc}", file=sys.stderr)
return 1
```
</issue_to_address>
### Comment 2
<location> `lading/commands/bump.py:10` </location>
<code_context>
+from plumbum import local
+
+
+def run(workspace_root: Path) -> str:
+ """Return a placeholder message for the bump command.
+
</code_context>
<issue_to_address>
**suggestion:** Redundant use of plumbum.local.path for workspace_root normalization.
Path normalization for workspace_root is duplicated in bump.py and publish.py, while _normalise_workspace_root in cli.py already handles this. Centralize this logic to prevent inconsistencies.
Suggested implementation:
```python
```
```python
def run(workspace_root: Path) -> str:
"""Return a placeholder message for the bump command.
```
Make sure that wherever `run()` is called (likely in your CLI entrypoint), you use `_normalise_workspace_root` from `cli.py` to normalize the path before passing it to `run()`. This will centralize the normalization logic and prevent duplication.
</issue_to_address>
### Comment 3
<location> `lading/commands/publish.py:10` </location>
<code_context>
+from plumbum import local
+
+
+def run(workspace_root: Path) -> str:
+ """Return a placeholder message for the bump command.
+
</code_context>
<issue_to_address>
**suggestion:** Redundant workspace_root normalization logic duplicated from bump.py.
Refactor the normalization logic into a shared utility to avoid duplication and enhance maintainability.
Suggested implementation:
```python
from plumbum import local
from lading.utils.path import normalize_workspace_root
def run(workspace_root: Path) -> str:
"""Return a placeholder message for the publish command."""
candidate = normalize_workspace_root(workspace_root)
```
You will need to implement the `normalize_workspace_root` function in `lading/utils/path.py` and refactor any other usages (such as in `bump.py`) to use this shared utility.
</issue_to_address>
### Comment 4
<location> `tests/unit/test_cli.py:48-51` </location>
<code_context>
+ assert remaining == expected_remaining
+
+
+def test_extract_workspace_override_requires_value() -> None:
+ """Require a value whenever ``--workspace-root`` appears."""
+ with pytest.raises(SystemExit):
+ cli._extract_workspace_override(["--workspace-root"])
+
</code_context>
<issue_to_address>
**suggestion (testing):** Missing test for '--workspace-root=' with no value.
Add a test to ensure that '--workspace-root=' without a value triggers SystemExit, matching the implementation's behavior.
```suggestion
def test_extract_workspace_override_requires_value() -> None:
"""Require a value whenever ``--workspace-root`` appears."""
with pytest.raises(SystemExit):
cli._extract_workspace_override(["--workspace-root"])
def test_extract_workspace_override_requires_value_equals() -> None:
"""Require a value whenever ``--workspace-root=`` appears with no value."""
with pytest.raises(SystemExit):
cli._extract_workspace_override(["--workspace-root="])
```
</issue_to_address>
### Comment 5
<location> `tests/unit/test_cli.py:54-73` </location>
<code_context>
+ cli._extract_workspace_override(["--workspace-root"])
+
+
+def test_main_dispatches_bump(
+ monkeypatch: pytest.MonkeyPatch,
+ tmp_path: Path,
+ capsys: pytest.CaptureFixture[str],
+) -> None:
+ """Route the bump subcommand through the placeholder implementation."""
+ called: dict[str, Path] = {}
+
+ def fake_run(workspace_root: Path) -> str:
+ called["workspace_root"] = workspace_root
+ return "bump placeholder"
+
+ monkeypatch.setattr(bump_command, "run", fake_run)
+ exit_code = cli.main(["--workspace-root", str(tmp_path), "bump"])
+ assert exit_code == 0
+ assert called["workspace_root"] == tmp_path.resolve()
+ captured = capsys.readouterr()
+ assert "bump placeholder" in captured.out
+
+
</code_context>
<issue_to_address>
**suggestion (testing):** No test for missing subcommand or invalid subcommand.
Please add tests to cover scenarios where the CLI is invoked without a subcommand or with an invalid subcommand, verifying that appropriate error handling and exit codes are produced.
```suggestion
def test_main_dispatches_bump(
monkeypatch: pytest.MonkeyPatch,
tmp_path: Path,
capsys: pytest.CaptureFixture[str],
) -> None:
"""Route the bump subcommand through the placeholder implementation."""
called: dict[str, Path] = {}
def fake_run(workspace_root: Path) -> str:
called["workspace_root"] = workspace_root
return "bump placeholder"
monkeypatch.setattr(bump_command, "run", fake_run)
exit_code = cli.main(["--workspace-root", str(tmp_path), "bump"])
assert exit_code == 0
assert called["workspace_root"] == tmp_path.resolve()
captured = capsys.readouterr()
assert "bump placeholder" in captured.out
def test_main_missing_subcommand(
capsys: pytest.CaptureFixture[str],
tmp_path: Path,
) -> None:
"""Test CLI invoked without a subcommand."""
exit_code = cli.main(["--workspace-root", str(tmp_path)])
assert exit_code != 0
captured = capsys.readouterr()
assert "No subcommand provided" in captured.out or "error" in captured.out.lower()
def test_main_invalid_subcommand(
capsys: pytest.CaptureFixture[str],
tmp_path: Path,
) -> None:
"""Test CLI invoked with an invalid subcommand."""
exit_code = cli.main(["--workspace-root", str(tmp_path), "not_a_real_subcommand"])
assert exit_code != 0
captured = capsys.readouterr()
assert "Unknown subcommand" in captured.out or "error" in captured.out.lower()
```
</issue_to_address>
### Comment 6
<location> `tests/bdd/steps/test_cli_steps.py:24-33` </location>
<code_context>
+ return tmp_path
+
+
+@when("I invoke lading bump with that workspace", target_fixture="cli_run")
+def when_invoke_lading(
+ cmd_mox: CmdMox,
+ workspace_directory: Path,
+ repo_root: Path,
+) -> dict[str, typ.Any]:
+ """Execute the CLI via ``python -m`` and capture the result."""
+ command = [
+ sys.executable,
+ "-m",
+ "lading.cli",
+ "--workspace-root",
+ str(workspace_directory),
+ "bump",
+ ]
+ cmd_mox.spy(sys.executable).passthrough()
+ cmd_mox.spy(Path(sys.executable).name).passthrough()
+ # ``cmd-mox`` intercepts the invocation and only executes the command we
+ # configure in this test module. We therefore silence Ruff's security check
+ # that normally warns about untrusted input.
+ completed = subprocess.run( # noqa: S603
+ command,
+ check=False,
</code_context>
<issue_to_address>
**suggestion (testing):** No behaviour test for the 'publish' subcommand.
Please add BDD tests for the 'publish' subcommand, including scenarios and step definitions, to ensure both CLI entry points are tested.
Suggested implementation:
```python
@when("I invoke lading bump with that workspace", target_fixture="cli_run")
def when_invoke_lading(
cmd_mox: CmdMox,
workspace_directory: Path,
repo_root: Path,
) -> dict[str, typ.Any]:
"""Execute the CLI via ``python -m`` and capture the result."""
command = [
sys.executable,
"-m",
"lading.cli",
"--workspace-root",
str(workspace_directory),
"bump",
]
cmd_mox.spy(sys.executable).passthrough()
cmd_mox.spy(Path(sys.executable).name).passthrough()
# ``cmd-mox`` intercepts the invocation and only executes the command we
# configure in this test module. We therefore silence Ruff's security check
# that normally warns about untrusted input.
completed = subprocess.run( # noqa: S603
command,
check=False,
cwd=str(repo_root),
capture_output=True,
text=True,
)
return {
"returncode": completed.returncode,
"stdout": completed.stdout,
"stderr": completed.stderr,
"workspace": workspace_directory.resolve(),
}
@when("I invoke lading publish with that workspace", target_fixture="cli_run")
def when_invoke_lading_publish(
cmd_mox: CmdMox,
workspace_directory: Path,
repo_root: Path,
) -> dict[str, typ.Any]:
"""Execute the CLI via ``python -m`` and capture the result for the publish subcommand."""
command = [
sys.executable,
"-m",
"lading.cli",
"--workspace-root",
str(workspace_directory),
"publish",
]
cmd_mox.spy(sys.executable).passthrough()
cmd_mox.spy(Path(sys.executable).name).passthrough()
completed = subprocess.run( # noqa: S603
command,
check=False,
cwd=str(repo_root),
capture_output=True,
text=True,
)
return {
"returncode": completed.returncode,
"stdout": completed.stdout,
"stderr": completed.stderr,
"workspace": workspace_directory.resolve(),
}
```
You will also need to add corresponding scenarios for the 'publish' subcommand in your `features/cli.feature` file, and ensure any necessary @then step definitions are present to assert expected outcomes for 'publish'.
</issue_to_address>
### Comment 7
<location> `lading/cli.py:96` </location>
<code_context>
+ return 0
+
+
+def main(argv: typ.Sequence[str] | None = None) -> int:
+ """Entry point for ``python -m lading.cli``."""
+ if argv is None:
</code_context>
<issue_to_address>
**issue (review_instructions):** Add unit tests for the main() entry point and CLI argument parsing logic.
The CLI scaffolding introduces new logic for argument parsing, environment variable management, and command dispatch. While behavioural tests are present, there are no unit tests covering the main() function or its helpers. Add unit tests to verify argument extraction, environment variable handling, and error cases.
<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 8
<location> `lading/commands/bump.py:10` </location>
<code_context>
+from plumbum import local
+
+
+def run(workspace_root: Path) -> str:
+ """Return a placeholder message for the bump command.
+
</code_context>
<issue_to_address>
**issue (review_instructions):** Add unit tests for the bump.run() function.
The bump command implementation is new and should be covered by unit tests to ensure correct path normalization and output formatting. Behavioural tests alone are insufficient for this function's logic.
<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 9
<location> `lading/commands/publish.py:10` </location>
<code_context>
+from plumbum import local
+
+
+def run(workspace_root: Path) -> str:
+ """Return a placeholder message for the bump command.
+
</code_context>
<issue_to_address>
**issue (review_instructions):** Add unit tests for the publish.run() function.
The publish command implementation is new and should be covered by unit tests to ensure correct path normalization and output formatting. Behavioural tests alone are insufficient for this function's logic.
<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 10
<location> `docs/usage-guide.md:9` </location>
<code_context>
+
+## Installation and invocation
+
+The CLI ships with the repository. You can execute it directly with Python or
+via `uv`:
+
</code_context>
<issue_to_address>
**issue (review_instructions):** This sentence uses the second person pronoun "you", which should be avoided per the instructions.
Consider rephrasing to avoid "you". For example: "The CLI ships with the repository and can be executed directly with Python or via `uv`."
<details>
<summary>Review instructions:</summary>
**Path patterns:** `**/*.md`
**Instructions:**
Avoid 2nd person or 1st person pronouns ("I", "you", "we")
</details>
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix: Comment on lines +37 to +69 def _extract_workspace_override(
tokens: typ.Sequence[str],
) -> tuple[str | None, list[str]]:
"""Split ``--workspace-root`` from CLI tokens.
The flag can appear in either ``--workspace-root <path>`` or
``--workspace-root=<path>`` form. The last occurrence wins, matching
common CLI conventions. The returned token list can be passed directly
to :func:`cyclopts.App.__call__`.
"""
workspace: str | None = None
remainder: list[str] = []
index = 0
while index < len(tokens):
current_argument = tokens[index]
if current_argument == "--workspace-root":
try:
workspace = tokens[index + 1]
except IndexError as err:
raise SystemExit(WORKSPACE_ROOT_REQUIRED_MESSAGE) from err
if workspace.startswith("-"):
raise SystemExit(WORKSPACE_ROOT_REQUIRED_MESSAGE)
index += 2
continue
if current_argument.startswith("--workspace-root="):
workspace = current_argument.partition("=")[2]
if not workspace:
raise SystemExit(WORKSPACE_ROOT_REQUIRED_MESSAGE)
index += 1
continue
remainder.append(current_argument)
index += 1
return workspace, remainder❌ New issue: Bumpy Road Ahead |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
lading/commands/publish.py (1)
10-14: Enhance the docstring to match the bump command's clarity.The docstring for
publish.runis minimal compared tobump.run(shown in the code snippets), which explains the placeholder rationale and testing intent. For consistency and maintainability, consider providing the same level of detail here.Apply this diff to enhance the docstring:
def run(workspace_root: Path) -> str: - """Return a placeholder message for the publish command.""" + """Return a placeholder message for the publish command. + + Step 1.1 only wires the CLI, so we provide a friendly acknowledgement + instead of performing actual publication. The message makes it trivial + for tests to assert that dispatch occurred correctly without + constraining future behaviour. + """ candidate = local.path(str(workspace_root)) root_path = Path(str(candidate)).expanduser().resolve(strict=False) return f"publish placeholder invoked for {root_path}"lading/commands/bump.py (1)
18-19: Remove redundant path normalization.The
workspace_rootparameter is already normalized (expanded and resolved) bycli.py's_normalise_workspace_root()before being passed to this function. Re-normalizing it here is redundant. Additionally, the plumbum round-trip (Path -> str -> plumbum path -> str -> Path) serves no purpose.Simplify to:
- candidate = local.path(str(workspace_root)) - root_path = Path(str(candidate)).expanduser().resolve(strict=False) - return f"bump placeholder invoked for {root_path}" + return f"bump placeholder invoked for {workspace_root}"If you need to keep normalization for defensive coding, at minimum remove the plumbum round-trip:
- candidate = local.path(str(workspace_root)) - root_path = Path(str(candidate)).expanduser().resolve(strict=False) + root_path = workspace_root.expanduser().resolve(strict=False) return f"bump placeholder invoked for {root_path}"lading/cli.py (2)
28-34: Simplify path normalization by removing unnecessary plumbum round-trip.The plumbum conversion (
Path/str -> plumbum path -> str -> Path) is redundant here. The code converts to plumbum'sLocalPath, immediately converts back to string, then topathlib.Path, and usespathlibmethods for expansion and resolution. This achieves nothing thatpathlib.Pathcannot do directly.Simplify to use only
pathlib.Path:def _normalise_workspace_root(value: Path | str | None) -> Path: """Return an absolute workspace path with ``~`` expanded.""" if value is None: return Path.cwd().resolve() - candidate = local.path(str(value)) - expanded = Path(str(candidate)).expanduser() - return expanded.resolve(strict=False) + path = Path(value) + return path.expanduser().resolve(strict=False)This maintains identical behavior while being clearer and more efficient.
106-121: Avoid redundant normalization in command functions.Both
bump()andpublish()call_normalise_workspace_root()on parameters that may already be normalized. Additionally, the downstreamcommands.bump.run()andcommands.publish.run()perform yet another normalization (see earlier comment on bump.py lines 18-19).The normalization happens up to three times for a single invocation:
- In
main()(line 101) before setting the environment variable- In
bump()/publish()(lines 111, 120) before calling command runners- In the command runners themselves
Consider one of these approaches:
Option 1 (recommended): Remove normalization from command runners (
commands.bump.run,commands.publish.run) and trust that the CLI layer provides normalized paths.Option 2: Add a note documenting why defensive re-normalization is needed, if there's a use case for calling these commands programmatically with non-normalized paths.
📜 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 ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (14)
Makefile(1 hunks)docs/lading-design.md(1 hunks)docs/roadmap.md(1 hunks)docs/usage-guide.md(1 hunks)lading/__init__.py(1 hunks)lading/cli.py(1 hunks)lading/commands/__init__.py(1 hunks)lading/commands/bump.py(1 hunks)lading/commands/publish.py(1 hunks)pyproject.toml(2 hunks)tests/bdd/features/cli.feature(1 hunks)tests/bdd/steps/test_cli_steps.py(1 hunks)tests/conftest.py(1 hunks)tests/unit/test_cli.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (7)
**/*.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/__init__.pylading/commands/__init__.pylading/commands/bump.pytests/bdd/steps/test_cli_steps.pylading/cli.pylading/commands/publish.pytests/unit/test_cli.pytests/conftest.py
{README.md,docs/**}
📄 CodeRabbit inference engine (.rules/python-00.md)
Colocate documentation: keep README.md or a docs/ directory near reusable packages and include usage examples
Files:
docs/roadmap.mddocs/lading-design.mddocs/usage-guide.md
docs/**/*.md
📄 CodeRabbit inference engine (AGENTS.md)
docs/**/*.md: Use markdown files in docs/ as the knowledge base and source of truth for requirements, dependencies, and architectural decisions.
Proactively update relevant docs/ markdown when decisions, requirements, dependencies, or architecture change.
Files:
docs/roadmap.mddocs/lading-design.mddocs/usage-guide.md
**/*.md
📄 CodeRabbit inference engine (AGENTS.md)
**/*.md: Markdown files must pass markdownlint.
Markdown files containing Mermaid diagrams must pass nixie validation.
Files:
docs/roadmap.mddocs/lading-design.mddocs/usage-guide.md
{**/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/bdd/steps/test_cli_steps.pytests/unit/test_cli.pytests/conftest.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/bdd/steps/test_cli_steps.pytests/unit/test_cli.pytests/conftest.py
pyproject.toml
📄 CodeRabbit inference engine (.rules/python-00.md)
pyproject.toml: Enable Ruff for linting (replacing flake8, isort, pyflakes, etc.) and configure it
Use Ruff as the project formatter; let Ruff handle all formatting
Configure tools (Ruff, Pyright, Pytest) via pyproject.toml
Enforce strict mode in PyrightConfigure Ruff to enforce TRY, BLE, EM, LOG, N818, PERF203, and B017 in pyproject.toml
pyproject.toml: Use the PEP 621 [project] table with at least name and version defined
Include description and readme in [project]; set readme to the README file path (e.g., README.md)
Set requires-python in [project] to declare supported Python versions (e.g., >=3.10)
Specify license in [project] using license = { text = "" } or license = { file = "LICENSE" }
Provide authors with name and email in [project].authors
Use keywords and valid Trove classifiers in [project]
Declare runtime dependencies in [project].dependencies using PEP 508 specifiers
Group non-runtime deps under [project.optional-dependencies] (e.g., dev, docs)
Define CLI entry points under [project.scripts] (e.g., mycli = "pkg.cli:main")
Define GUI entry points under [project.gui-scripts] when needed
Register plugin entry points under [project.entry-points.'group.name']
Declare a build system: [build-system] requires = ["setuptools>=61.0", "wheel"], build-backend = "setuptools.build_meta"
Set [tool.uv].package = true to ensure your project is built/installed on uv sync/run
If omitting [build-system], set [tool.uv].package = true so uv still builds/installs your package
Use semantic versioning (e.g., 1.2.3) for the [project].version value
Keep build constraints minimal; omit [build-system] if you don’t need editable installs
Prefer exact or bounded dependency ranges (e.g., requests>=2.25,<3.0) to avoid unexpected major bumps
Use dynamic fields (e.g., dynamic = ["version"]) sparingly and only if the build backend supports them
Files:
pyproject.toml
🧬 Code graph analysis (7)
lading/__init__.py (1)
lading/cli.py (1)
main(96-103)
lading/commands/__init__.py (1)
lading/cli.py (2)
bump(107-112)publish(116-121)
lading/commands/bump.py (2)
lading/commands/publish.py (1)
run(10-14)crate_tools/unittests/test_publish_workspace_dependencies.py (1)
workspace_root(16-21)
tests/bdd/steps/test_cli_steps.py (2)
tests/conftest.py (1)
repo_root(15-17)lading/commands/bump.py (1)
run(10-20)
lading/cli.py (2)
lading/commands/bump.py (1)
run(10-20)lading/commands/publish.py (1)
run(10-14)
lading/commands/publish.py (2)
lading/commands/bump.py (1)
run(10-20)crate_tools/unittests/test_publish_workspace_dependencies.py (1)
workspace_root(16-21)
tests/unit/test_cli.py (1)
lading/cli.py (4)
bump(107-112)publish(116-121)_extract_workspace_override(37-69)main(96-103)
⏰ 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 (13)
docs/roadmap.md (1)
15-15: LGTM!The roadmap correctly reflects the completion of Step 1.1 tasks. The status updates align with the CLI scaffolding and project initialization delivered in this PR.
Also applies to: 20-20
docs/usage-guide.md (1)
1-64: LGTM!The usage guide is clear, accurate, and well-structured. It correctly documents:
- The CLI entry point and invocation methods
- The
--workspace-rootflag behavior and environment variable handling- Placeholder command implementations and future roadmap expectations
- Testing approach with cmd-mox
The documentation aligns well with the implementation in
lading/cli.pyand provides helpful context for both users and maintainers.pyproject.toml (1)
28-28: LGTM!The Pyright include and setuptools package configuration correctly expose the new
ladingpackage for type checking and distribution. The changes align with the new package structure introduced in this PR.Also applies to: 108-115
lading/__init__.py (1)
1-11: LGTM!The package initializer properly exposes the public API (
appandmain) with clear documentation. The implementation follows Python best practices with future annotations and explicit__all__exports.tests/bdd/features/cli.feature (1)
1-5: LGTM!The BDD feature provides appropriate smoke-test coverage for the CLI scaffolding. The scenario structure is clear and matches the documented behavior in the usage guide.
lading/commands/publish.py (1)
12-13: LGTM!The path normalization approach using
plumbum.localfollowed bypathlib.Pathresolution is consistent with thebumpcommand implementation. This ensures cross-platform path handling and proper resolution of symbolic links and relative paths.lading/commands/__init__.py (1)
1-7: LGTM!The commands package initializer properly exposes the command submodules with a clear docstring. The implementation follows Python packaging best practices and maintains consistency with the parent package structure.
docs/lading-design.md (1)
76-90: LGTM!The implementation notes accurately document the CLI scaffolding delivered in Step 1.1. The content:
- Clearly identifies the entry point and key implementation details
- Explains the workspace-root handling and environment variable pattern
- Documents the placeholder approach and testing strategy
- Provides valuable context for future roadmap steps
The documentation aligns well with the actual implementation in
lading/cli.pyand related test files.tests/conftest.py (1)
1-32: LGTM!The pytest configuration is well-structured:
- The
repo_rootfixture correctly derives the repository root- The
_restore_workspace_envautouse fixture properly isolates environment state using try/finally- Importing
WORKSPACE_ROOT_ENV_VARinside the fixture avoids import-time side effectstests/unit/test_cli.py (1)
1-97: LGTM!The test suite effectively validates the CLI scaffolding:
- Comprehensive parametrized tests for workspace override extraction
- Proper error handling validation using
pytest.raises- Appropriate use of monkeypatching to isolate dispatch logic
- Good coverage of both the main entry point and direct app invocation
tests/bdd/steps/test_cli_steps.py (1)
1-64: LGTM!The BDD step definitions are well-implemented:
- Clear separation of concerns across given/when/then steps
- Appropriate use of
cmd_moxfor subprocess interception- Justified use of
subprocess.runwith security linter suppression- Proper fixture wiring to pass context between steps
lading/cli.py (2)
37-69: LGTM!The manual token parsing for
--workspace-rootis robust:
- Correctly handles both
--workspace-root <value>and--workspace-root=<value>forms- Properly validates that values are not flag-like (don't start with
-)- Raises
SystemExitwith a clear error message for missing values- Honors last-occurrence-wins semantics
72-103: LGTM!The environment management and dispatch logic is well-designed:
_workspace_envcontext manager properly saves/restores environment state_dispatch_and_printhandles both int and string results appropriatelymain()cleanly orchestrates extraction, normalization, environment setup, and dispatch
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
lading/utils/path.py (1)
10-16: Consider simplifying the path conversion.The function converts the input to a plumbum path and immediately back to pathlib.Path. Since pathlib.Path already provides
expanduser()andresolve()natively, the plumbum conversion appears unnecessary.Apply this diff to simplify:
def normalise_workspace_root(value: Path | str | None) -> Path: """Return an absolute workspace path with ``~`` expanded.""" if value is None: return Path.cwd().resolve() - candidate = local.path(str(value)) - expanded = Path(str(candidate)).expanduser() - return expanded.resolve(strict=False) + return Path(value).expanduser().resolve(strict=False)If there's a specific reason for the plumbum conversion (e.g., handling platform-specific path formats), please document it in a comment.
lading/cli.py (1)
103-105: Clarify the early-exit logic for empty remaining tokens.When
remainingis empty, line 104 calls_dispatch_and_print(remaining)before returning 2 on line 105. This invokes the Cyclopts app with an empty argument list to display help, but the flow is implicit.Consider making the intent explicit:
workspace_root = normalise_workspace_root(workspace_override) if not remaining: - _dispatch_and_print(remaining) + _dispatch_and_print(remaining) # Show usage/help for empty command return 2 with _workspace_env(workspace_root):Or refactor to be more explicit about showing help:
workspace_root = normalise_workspace_root(workspace_override) if not remaining: - _dispatch_and_print(remaining) - return 2 + # No subcommand provided; display help and return error code + return _dispatch_and_print(remaining) or 2 with _workspace_env(workspace_root):
📜 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 (12)
docs/lading-design.md(1 hunks)docs/usage-guide.md(1 hunks)lading/cli.py(1 hunks)lading/commands/bump.py(1 hunks)lading/commands/publish.py(1 hunks)lading/utils/__init__.py(1 hunks)lading/utils/path.py(1 hunks)pyproject.toml(2 hunks)tests/bdd/features/cli.feature(1 hunks)tests/bdd/steps/test_cli_steps.py(1 hunks)tests/unit/test_cli.py(1 hunks)tests/unit/test_commands_placeholder.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- lading/commands/publish.py
- docs/usage-guide.md
- lading/commands/bump.py
- docs/lading-design.md
- pyproject.toml
🧰 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/utils/path.pytests/unit/test_commands_placeholder.pylading/cli.pytests/unit/test_cli.pytests/bdd/steps/test_cli_steps.pylading/utils/__init__.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_commands_placeholder.pytests/unit/test_cli.pytests/bdd/steps/test_cli_steps.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_commands_placeholder.pytests/unit/test_cli.pytests/bdd/steps/test_cli_steps.py
🧬 Code graph analysis (5)
tests/unit/test_commands_placeholder.py (4)
lading/cli.py (2)
bump(117-122)publish(126-131)lading/utils/path.py (1)
normalise_workspace_root(10-16)lading/commands/bump.py (1)
run(13-22)lading/commands/publish.py (1)
run(13-16)
lading/cli.py (3)
lading/utils/path.py (1)
normalise_workspace_root(10-16)lading/commands/bump.py (1)
run(13-22)lading/commands/publish.py (1)
run(13-16)
tests/unit/test_cli.py (3)
lading/cli.py (5)
bump(117-122)publish(126-131)_extract_workspace_override(28-60)main(96-113)_workspace_env(64-74)lading/utils/path.py (1)
normalise_workspace_root(10-16)crate_tools/unittests/test_publish_workspace_dependencies.py (1)
workspace_root(16-21)
tests/bdd/steps/test_cli_steps.py (3)
tests/conftest.py (1)
repo_root(15-17)lading/commands/bump.py (1)
run(13-22)lading/commands/publish.py (1)
run(13-16)
lading/utils/__init__.py (1)
lading/utils/path.py (1)
normalise_workspace_root(10-16)
⏰ 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
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix: Comment on lines +72 to +89 def test_main_dispatches_bump(
monkeypatch: pytest.MonkeyPatch,
tmp_path: Path,
capsys: pytest.CaptureFixture[str],
) -> None:
"""Route the bump subcommand through the placeholder implementation."""
called: dict[str, Path] = {}
def fake_run(workspace_root: Path) -> str:
called["workspace_root"] = workspace_root
return "bump placeholder"
monkeypatch.setattr(bump_command, "run", fake_run)
exit_code = cli.main(["--workspace-root", str(tmp_path), "bump"])
assert exit_code == 0
assert called["workspace_root"] == tmp_path.resolve()
captured = capsys.readouterr()
assert "bump placeholder" in captured.out❌ New issue: Code Duplication |
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix: Comment on lines +90 to +113 def test_main_dispatches_command(
monkeypatch: pytest.MonkeyPatch,
tmp_path: Path,
capsys: pytest.CaptureFixture[str],
command_module: ModuleType,
command_name: str,
placeholder_text: str,
cli_args: list[str],
) -> None:
"""Route subcommands through their placeholder implementations."""
called: dict[str, Path] = {}
def fake_run(workspace_root: Path) -> str:
called["workspace_root"] = workspace_root
return placeholder_text
monkeypatch.setattr(command_module, "run", fake_run)
args = [arg.replace("{tmp_path}", str(tmp_path)) for arg in cli_args]
assert command_name in args
exit_code = cli.main(args)
assert exit_code == 0
assert called["workspace_root"] == tmp_path.resolve()
captured = capsys.readouterr()
assert placeholder_text in captured.out❌ New issue: Excess Number of Function Arguments Comment on lines +143 to +160 def test_main_handles_exceptions(
monkeypatch: pytest.MonkeyPatch,
capsys: pytest.CaptureFixture[str],
tmp_path: Path,
exception: BaseException,
expected_exit_code: int,
expected_message: str,
) -> None:
"""Handle exceptions during command execution."""
def boom(_: typ.Sequence[str]) -> int:
raise exception
monkeypatch.setattr(cli, "_dispatch_and_print", boom)
exit_code = cli.main(["bump", "--workspace-root", str(tmp_path)])
assert exit_code == expected_exit_code
captured = capsys.readouterr()
assert expected_message in captured.err❌ New issue: Excess Number of Function Arguments |
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
lading/cli.py (1)
118-120: Consider adding a clarifying comment for the usage-error path.The code intentionally calls
_dispatch_and_print(remaining)to trigger Cyclopts' usage message, then unconditionally returns exit code 2 (standard for CLI usage errors). The dispatch call's return value is ignored.While the behavior is correct, a brief inline comment would clarify the intent:
if not remaining: + # Dispatch with empty args to print usage, then return standard usage-error code _dispatch_and_print(remaining) return 2Alternatively, make the intention more explicit:
if not remaining: - _dispatch_and_print(remaining) - return 2 + _dispatch_and_print(remaining) # Print usage message + return 2 # Standard exit code for missing subcommand
📜 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 (2)
lading/cli.py(1 hunks)tests/unit/test_cli.py(1 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/cli.pytests/unit/test_cli.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_cli.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_cli.py
🧬 Code graph analysis (2)
lading/cli.py (3)
lading/utils/path.py (1)
normalise_workspace_root(10-16)lading/commands/publish.py (1)
run(13-16)lading/commands/bump.py (1)
run(13-22)
tests/unit/test_cli.py (2)
lading/cli.py (5)
bump(132-137)publish(141-146)_extract_workspace_override(52-75)main(111-128)_workspace_env(79-89)lading/utils/path.py (1)
normalise_workspace_root(10-16)
🪛 GitHub Actions: CI
tests/unit/test_cli.py
[error] 3-14: I001 Import block is un-sorted or un-formatted. Organize imports.
[error] 6-6: ICN003 Members of dataclasses should not be imported explicitly.
⏰ 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 (5)
tests/unit/test_cli.py (3)
110-114: Dataclass bundling successfully reduces argument count to guideline limit.The parametrized tests correctly use dataclasses (
CommandDispatchCase,ExceptionHandlingCase) to bundle test parameters, keeping function argument counts at exactly 4 (the guideline maximum). CodeScene may be incorrectly counting the dataclass fields as separate function parameters.The refactor suggested in the PR objectives has been properly applied. The test structure is clean and follows pytest best practices.
133-141: LGTM: Missing subcommand case is properly tested.The test correctly validates that invoking the CLI without a subcommand returns exit code 2 and displays usage information.
143-151: LGTM: Invalid subcommand case is properly tested.The test correctly validates that invoking the CLI with an unknown subcommand returns a non-zero exit code and displays an error message.
lading/cli.py (2)
28-75: Refactoring successfully addresses nested conditional complexity.The extraction of
_validate_workspace_value,_parse_workspace_flag, and_parse_workspace_equalshelper functions has successfully flattened the logic in_extract_workspace_override, eliminating the "Bumpy Road Ahead" issue flagged by CodeScene in previous reviews.The refactor maintains identical behavior and error messages while improving readability and reducing cyclomatic complexity, aligning with the coding guideline: "Prefer clear, linear data flows over deeply nested conditionals and loop bodies."
Based on past review comments indicating this refactor was completed in commit 8ba2313.
131-146: LGTM: Command definitions follow consistent patterns.Both
bumpandpublishsubcommands correctly:
- Use the
WorkspaceRootOptiontype alias for parameter annotation- Normalize the workspace root via
normalise_workspace_root- Delegate to their respective command module
runfunctions- Return string results for display by the dispatcher
The implementation provides clean separation between CLI wiring and command logic, making it easy to replace placeholder implementations with real functionality.
1 participant
Summary
Testing
https://chatgpt.com/codex/tasks/task_e_68e1b348fb0883229d1ca0883fd0d0c8
Summary by Sourcery
Add initial scaffolding for the lading CLI toolkit with cyclopts, including placeholder bump and publish commands, comprehensive tests, and supporting documentation.
New Features:
Enhancements:
Documentation:
Tests:
Summary by CodeRabbit
New Features
Documentation
Utilities
Tests
Chores