Migrate command execution to Cuprum: introduce shared catalogue - #51
Conversation
Establish the foundation for the Cuprum migration by creating a shared programme catalogue in lading/utils/commands.py. The catalogue registers cargo and git as allowed executables using cuprum's ProgramCatalogue. Implementation details: - Create LADING_CATALOGUE with CARGO and GIT program constants - Export catalogue and constants from lading.utils package - Add unit tests verifying catalogue construction and allowlist behaviour - Add BDD scenarios for command construction in scoped contexts - Update docs/lading-design.md with actual cuprum 0.1.0 API patterns - Mark "Define Lading Catalogue" task as done in roadmap The cuprum 0.1.0 API uses ProgramCatalogue, Program, and ProjectSettings rather than the Catalogue.from_programs() pattern described in earlier documentation. Section 7.3 of lading-design.md has been updated to reflect the actual implementation. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
|
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. Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughMigrate command execution to a Cuprum ProgramCatalogue: add public constants CARGO, GIT and LADING_CATALOGUE; update docs and roadmap text; add BDD and unit tests verifying catalogue registration, scoped command construction, argv formation and UnknownProgramError handling. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Tester
participant Scoped as Cuprum.scoped()
participant Catalogue as LADING_CATALOGUE
participant Builder as CommandBuilder
participant Executor as Shell / cmd-mox
Tester->>Scoped: enter scoped(allowlist=Catalogue.allowlist)
note right of Scoped `#f0f4c3`: Scoped context provides\nallowlist and builder factories
Scoped->>Builder: request builder for Program (CARGO/GIT)
Builder->>Builder: assemble argv_with_program from Program + args
Builder->>Executor: hand off constructed command
Executor-->>Tester: return CommandResult / raise UnknownProgramError
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (3)**/*.py📄 CodeRabbit inference engine (AGENTS.md)
Files:
⚙️ CodeRabbit configuration file
Files:
**/test_*.py📄 CodeRabbit inference engine (.rules/python-00.md)
Files:
**/*test*.py📄 CodeRabbit inference engine (.rules/python-exception-design-raising-handling-and-logging.md)
Files:
🔍 Remote MCP DeepwikiSummary of additional repository facts relevant to this PR (concise, review-focused)
Sources
⏰ Context from checks skipped due to timeout of 120000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
🔇 Additional comments (1)
Comment |
Reviewer's GuideIntroduces a shared Cuprum-based command execution catalogue ( Sequence diagram for scoped command execution using LADING_CATALOGUEsequenceDiagram
actor Developer
participant ClientCode
participant CommandsModule
participant CuprumScoped as ScopedContext
participant CuprumSh as ShFactory
participant Catalogue as ProgramCatalogue
participant Cargo as ExternalCargo
Developer->>ClientCode: import CARGO, LADING_CATALOGUE
Developer->>ClientCode: with scoped(allowlist=LADING_CATALOGUE.allowlist)
ClientCode->>CommandsModule: access CARGO
ClientCode->>ScopedContext: enter scoped(allowlist)
ScopedContext->>Catalogue: enforce allowlist
ClientCode->>ShFactory: make(CARGO, catalogue=LADING_CATALOGUE)
ShFactory->>Catalogue: validate program CARGO
Catalogue-->>ShFactory: validation OK
ShFactory-->>ClientCode: cargo_builder
ClientCode->>Cargo: cargo_builder("metadata", "--format-version", "1").run_sync()
Cargo-->>ClientCode: CommandResult
Class diagram for the new Cuprum command catalogue in lading.utils.commandsclassDiagram
class CommandsModule {
+Program CARGO
+Program GIT
+ProgramCatalogue LADING_CATALOGUE
}
class Program {
+str name
}
class ProjectSettings {
+str name
+tuple programs
+tuple documentation_locations
+tuple noise_rules
}
class ProgramCatalogue {
+tuple projects
+Allowlist allowlist
}
CommandsModule --> Program : uses
CommandsModule --> ProjectSettings : configures
CommandsModule --> ProgramCatalogue : exposes
ProjectSettings "1" --> "many" Program : registers
ProgramCatalogue "1" --> "many" ProjectSettings : aggregates
Flow diagram for command invocation via cmd-mox stub vs Cuprum catalogueflowchart TD
Caller["Caller code (publishing, preflight, etc.)"] --> Invoke["_invoke(program, args, cwd)"]
Invoke -->|LADING_USE_CMD_MOX_STUB is true| CmdMox["_invoke_via_cmd_mox(program, args, cwd)"]
Invoke -->|LADING_USE_CMD_MOX_STUB is false| Scoped["cuprum.scoped(allowlist=LADING_CATALOGUE.allowlist)"]
Scoped --> MakeCmd["sh.make(program, catalogue=LADING_CATALOGUE)"]
MakeCmd --> Builder["cmd_builder(*args)"]
Builder --> RunSync["cmd.run_sync() (Cuprum CommandResult)"]
CmdMox --> IPCServer["cmd-mox IPC server"]
RunSync --> ExternalProc["Real external process (cargo or git)"]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![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 Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix: tests/bdd/steps/test_commands_catalogue_steps.py Comment on lines +53 to +63 def when_construct_cargo_command(
catalogue_context: ProgramCatalogue,
args: str,
) -> SafeCmd:
"""Construct a cargo command with the given arguments."""
from cuprum import scoped, sh
parsed_args = _parse_quoted_args(args)
with scoped(allowlist=catalogue_context.allowlist):
cargo_builder = sh.make(CARGO, catalogue=catalogue_context)
return cargo_builder(*parsed_args)❌ New issue: Code Duplication |
This comment was marked as resolved.
This comment was marked as resolved.
Extract the duplicated command construction logic from when_construct_cargo_command and when_construct_git_command into a shared _construct_command_with_args helper function. This eliminates code duplication while preserving the same test coverage. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
leynos
changed the title
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
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: 2
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (11)
docs/developers-guide.mddocs/lading-design.mddocs/roadmap.mddocs/scripting-standards.mddocs/users-guide.mdlading/utils/__init__.pylading/utils/commands.pypyproject.tomltests/bdd/features/commands_catalogue.featuretests/bdd/steps/test_commands_catalogue_steps.pytests/unit/utils/test_commands.py
🧰 Additional context used
📓 Path-based instructions (7)
docs/**/*.md
📄 CodeRabbit inference engine (AGENTS.md)
docs/**/*.md: Use the markdown files within thedocs/directory as a knowledge base and source of truth for project requirements, dependency choices, and architectural decisions.
Proactively update relevant files in thedocs/directory when new decisions are made, requirements change, libraries are added/removed, or architectural patterns evolve to keep documentation accurate and current.
docs/**/*.md: Use British English based on the Oxford English Dictionary (en-GB-oxendict) with suffixes: -ize in words like 'realize' and 'organization', -lyse in words like 'analyse' and 'paralyse', -our in words like 'colour' and 'behaviour', -re in words like 'centre' and 'calibre', double 'l' in words like 'cancelled' and 'counsellor', maintain 'e' in words like 'likeable', -ogue in words like 'catalogue'
Use the Oxford comma in documentation: 'ships, planes, and hovercraft' where it aids comprehension
Treat company names as collective nouns in documentation, for example 'Lille Industries are expanding'
Write headings in sentence case
Use Markdown headings (#, ##, ###, and so on) in order without skipping levels
Follow markdownlint recommendations for Markdown formatting
Always provide a language identifier for fenced code blocks; use 'plaintext' for non-code text
Use '-' as the first level bullet and renumber lists when items change in Markdown
Prefer inline links using text or angle brackets around the URL in Markdown
Ensure blank lines before and after bulleted lists and fenced blocks in Markdown
Ensure tables have a delimiter line below the header row in Markdown
Expand any uncommon acronym on first use, for example, Continuous Integration (CI)
Wrap paragraphs at 80 columns in documentation
Wrap code at 120 columns in documentation
Do not wrap tables in documentation
Use footnotes referenced with [^label] in Markdown documentation
Include Mermaid diagrams in documentation where they add clarity
When embedding figures in documentation, use 
**/*.md: For Markdown files (.md only), ensure changes pass lint checks viamake markdownlint.
For Markdown files (.md only), ensure Mermaid diagrams pass validation using nixie viamake nixie.
Files:
docs/developers-guide.mddocs/users-guide.mddocs/roadmap.mddocs/lading-design.mddocs/scripting-standards.md
⚙️ CodeRabbit configuration file
**/*.md: * Avoid 2nd person or 1st person pronouns ("I", "you", "we")
- Use en-GB-oxendict (-ize / -yse / -our) spelling and grammar
- Headings must not be wrapped.
- Documents must start with a level 1 heading
- Headings must correctly increase or decrease by no more than one level at a time
- Use GitHub-flavoured Markdown style for footnotes and endnotes.
- Numbered footnotes must be numbered by order of appearance in the document.
Files:
docs/developers-guide.mddocs/users-guide.mddocs/roadmap.mddocs/lading-design.mddocs/scripting-standards.md
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: For Python files, ensure changes pass all relevant unit and behavioral tests according to testing guidelines.
For Python files, ensure changes pass lint checks viamake lint.
For Python files, ensure changes adhere to formatting standards viamake check-fmt.
For Python files, ensure changes pass type checking viamake typecheck.
For Python development, refer to detailed guidelines in the.rules/directory including Python Code Style Guidelines, Context Managers, Exception Design/Raising/Handling/Logging, Generators, Project Configuration, Return Patterns, and Typing best practices.
**/*.py: Use snake_case.py for Python file names, naming them for their contents
Use PascalCase for class names
Use snake_case for variable and function names
Use UPPER_SNAKE_CASE for module-level constants
Prefix private or internal helpers with a single underscore (_)
Enable and maintain full static type coverage using Pyright; use typing everywhere
Use TypedDict or @DataClass(slots=True) for structured data
Avoid Any; use precise types (TypeVar, Protocol, Literal, Union) and typing.cast only when necessary with justification
Be explicit with return type annotations using -> None, -> str, etc., for all public functions and class methods
Prefer immutability: use tuples over lists and types.MappingProxyType for read-only mappings
Enforce strict mode in Pyright; treat all Pyright warnings as CI errors and use # pyright: ignore sparingly with explanation
Avoid side effects at import time; modules should not modify global state or perform actions on import
Document public functions, classes, and modules using NumPy-format docstrings
Use inline comments to explain tricky code and non-obvious logic
**/*.py: Use context managers (viacontextlib.contextmanageror__enter__/__exit__methods) to encapsulate setup and teardown logic for resource management (files, locks, connections, etc.)
Use@contextlib.contextmanagerdecorator for straightforward procedural setup...
Files:
lading/utils/commands.pylading/utils/__init__.pytests/unit/utils/test_commands.pytests/bdd/steps/test_commands_catalogue_steps.py
⚙️ CodeRabbit configuration file
**/*.py: - Keep C90 / mccabe complexity ≤ 9
- Follow single responsibility and CQRS (command/query segregation)
- Prefer structural pattern matching to
- Prefer structural pattern matching over
isinstance()or imperative decomposition.- Docstrings must follow the
numpystyle guide. Use a single-line summary for private functions and methods, and full structured docs for all public interfaces.- Move conditionals with >2 branches to predicate/helper functions
- Avoid
eval,exec,pickle, monkey-patching,ctypes, unsafe shell- Every module must begin with a triple-quoted docstring explaining its purpose, utility, and usage, including example calls if appropriate.
- Comments and docs must follow en-GB-oxendict (-ize / -yse / -our) spelling and grammar
- Lint suppressions:
- Blanket
# noqa, file-level skips, and categories are forbidden- Only narrow in-line disables (
# noqa: XYZ) are permitted, and must be accompanied byFIXME:or a ticket link, and used only as a last resort.- Use
pytestfixtures for shared setup (conftest.pyorfixtures/)- Replace duplicate tests with
@pytest.mark.parametrize- Prefer
pytest-mockorunittest.mockfor stubs/mocks- Use
assert …, "message"over bare asserts- Reflect all API/behaviour changes in
docs/and update roadmap on completion- Files must not exceed 400 logical lines:
- Decompose large modules into subpackages
- Split large
match/caseor dispatch tables by domain and collocate with targets if appropriate- Move bulky data (fixtures, templates) to external files for parsing at runtime
- Mutable defaults and shadowed built-ins are forbidden
- All code must have clear type hints using modern style (
A | B,list[str],class Foo[A]:,type Bar = int, etc.), with ABC imports drawn from the correct stdlib module.- All path manipulation must be performed using pathlib for cross platform safety. Do not use string manipulation for extraction of path components or filename ele...
Files:
lading/utils/commands.pylading/utils/__init__.pytests/unit/utils/test_commands.pytests/bdd/steps/test_commands_catalogue_steps.py
pyproject.toml
📄 CodeRabbit inference engine (.rules/python-00.md)
Configure tools like Ruff, Pyright, and Pytest using pyproject.toml
pyproject.toml: Use PEP 621[project]table for metadata (name, version, description, readme, requires-python, license, authors, keywords, classifiers) and runtime dependencies
Include mandatory PEP 621 fields:nameandversionin the[project]table
Include recommended[project]metadata fields:description,readme(pointing to README.md),requires-python(e.g., >=3.10),license,authors,keywords, andclassifiers
Declare runtime dependencies as a list in PEP 508 format within the[project]tabledependenciesfield (e.g., "requests>=2.25")
Use[project.optional-dependencies]to group development and documentation dependencies separately from production dependencies
Define console entry points in[project.scripts]table and GUI entry points in[project.gui-scripts]table to expose CLIs or GUIs
Declare[build-system]table withrequires = ["setuptools>=61.0", "wheel"]andbuild-backend = "setuptools.build_meta"to support editable installs
Set[tool.uv]withpackage = trueto ensureuv syncbuilds and installs your project into its virtual environment
Keeppyproject.tomlhuman-readable by editing it by hand when possible and using TOML-aware editors
Declaredynamic = ["version"]sparingly; only use it when your version is computed at build time (e.g., via setuptools_scm), and ensure your build backend supports dynamic metadata
Keep build system constraints minimal; omit[build-system]if you don't need editable installs, but settool.uv.package = trueto override default behavior
Files:
pyproject.toml
**/test_*.py
📄 CodeRabbit inference engine (.rules/python-00.md)
**/test_*.py: Use pytest idioms: prefer fixtures over setup/teardown, parametrize broadly, avoid unnecessary mocks
Group related tests using class with method names prefixed by test_
Write tests from a user's perspective; test public behavior, not internals
Avoid mocking too much; prefer test doubles only for external services or non-deterministic behaviors
Files:
tests/unit/utils/test_commands.pytests/bdd/steps/test_commands_catalogue_steps.py
**/*test*.py
📄 CodeRabbit inference engine (.rules/python-exception-design-raising-handling-and-logging.md)
In tests, use narrow exception assertions with
pytest.raises()specifying the expected type and optionally constraining the message via regex (B017)
Files:
tests/unit/utils/test_commands.pytests/bdd/steps/test_commands_catalogue_steps.py
docs/scripting-standards.md
📄 CodeRabbit inference engine (docs/roadmap.md)
Update scripting standards documentation to feature cuprum as the standard for command execution, replacing plumbum patterns with cuprum equivalents
Files:
docs/scripting-standards.md
🧬 Code graph analysis (1)
lading/utils/__init__.py (1)
lading/utils/path.py (1)
normalise_workspace_root(10-16)
🪛 LanguageTool
docs/users-guide.md
[grammar] ~153-~153: This phrase is duplicated. You should probably use “array of” only once.
Context: ... | | aux_build | array of array of strings | [] | Extra commands (tok...
(PHRASE_REPETITION)
🔍 Remote MCP
Let me try a web search to gather information about Cuprum and best practices for command execution:Let me search specifically for Cuprum Python package and its documentation:Let me try a more specific search for Cuprum as a Rust/Python command execution library:Based on my search attempts, I was unable to locate specific online documentation for the Cuprum library. The searches returned general information about command injection security and other unrelated packages. Let me provide a summary based on the context provided in the PR and general best practices:
Summary of Relevant Context for PR Review
Based on the pull request context provided, here are the key facts relevant to your review:
PR Focus & Architecture
This PR introduces a Cuprum-based command execution foundation with the following key components:
- New module:
lading/utils/commands.pyestablishes a centralized, type-safe allowlist catalogue using Cuprum'sProgram,ProjectSettings, andProgramCatalogueabstractions - Public exports: Three constants (
CARGO,GIT,LADING_CATALOGUE) are now exposed vialading/utils/__init__.pyfor broader accessibility - Dependency: Cuprum is added as a new runtime dependency in
pyproject.toml
Security & Safety Pattern
The PR implements an allowlist-based command execution pattern. According to general secure command execution best practices, using an allowlist for inputs provides important security protection against command injection vulnerabilities. The implementation:
- Registers only
cargoandgitas allowed external executables - Requires explicit catalogue context (
cuprum.scoped) for command construction - Raises
UnknownProgramErrorwhen attempting to construct commands for unregistered programs
Test Coverage
The PR includes comprehensive test coverage:
- Unit tests (
tests/unit/utils/test_commands.py): Validate catalogue registration, allowlist membership, lookup behavior, and error handling - BDD tests (
tests/bdd/features/commands_catalogue.featureandtests/bdd/steps/test_commands_catalogue_steps.py): Verify command construction within scoped contexts and proper error handling for unregistered programs
Code Duplication Issue
The reviewer flagged duplicated code in tests/bdd/steps/test_commands_catalogue_steps.py:
- Two functions (
when_construct_cargo_commandandwhen_construct_git_command) share identical logic - Proposed solution: Extract common logic into a private
_construct_command_with_argshelper function to eliminate duplication while preserving test behavior and coverage
Migration Context
The PR represents Step 5.1 of an incremental migration from Plumbum to Cuprum, with documentation updates reflecting the new catalogue-driven execution model and scoped allowlist pattern. Test harness preservation ensures end-to-end testing continues via cmd-mox.
⏰ Context from checks skipped due to timeout of 120000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Sourcery review
- GitHub Check: Sourcery review
🔇 Additional comments (22)
docs/scripting-standards.md (3)
35-39: LGTM: Clear Cuprum integration guidance.The updated description correctly positions Cuprum as the standard for external process invocation with allowlist-based security. The wording is concise and aligns with the broader migration narrative.
154-156: LGTM: Appropriate distinction between shared and local catalogues.The guidance correctly advises standalone scripts and tests to define local catalogues, avoiding coupling to the main application's LADING_CATALOGUE. This promotes modularity and test isolation.
528-530: LGTM: Critical semantic difference highlighted.The migration note correctly emphasises that Cuprum's
run_sync()returnsCommandResultwithout raising exceptions, requiring explicitresult.exit_codechecks. This is a critical behavioural change that developers must understand when porting from Plumbum.docs/developers-guide.md (1)
22-26: LGTM: Clearer description of cmd-mox IPC behaviour.The rewording improves readability whilst preserving the semantic meaning about stubbed cargo invocations in end-to-end tests.
docs/roadmap.md (1)
280-285: LGTM: Accurate completion status.Step 5.1 is correctly marked as complete. The LADING_CATALOGUE, CARGO, and GIT constants are now defined in
lading/utils/commands.pyand verified by unit and BDD tests.docs/users-guide.md (1)
34-35: LGTM: Corrected wording.The merge of "one or" and "more" into "one or more" fixes the grammatical error.
tests/bdd/features/commands_catalogue.feature (1)
1-27: LGTM: Comprehensive BDD coverage for the command catalogue.The feature file provides excellent coverage:
- Registration verification for cargo and git
- Command construction within scoped contexts
- Error handling for unregistered programmes
The scenarios are clearly written and align with the Cuprum-based execution model introduced in this PR.
lading/utils/__init__.py (1)
5-8: LGTM: Clean public API expansion.The re-exports of CARGO, GIT, and LADING_CATALOGUE follow Python packaging conventions and provide convenient access to the shared catalogue infrastructure.
tests/bdd/steps/test_commands_catalogue_steps.py (4)
18-20: LGTM: Clean argument parsing utility.The regex-based parsing of quoted arguments is appropriate for the BDD test context where arguments are passed as strings.
23-34: Excellent refactoring: duplication eliminated.The
_construct_command_with_argshelper successfully extracts the common logic fromwhen_construct_cargo_commandandwhen_construct_git_command, as described in the PR objectives. This reduces duplication whilst preserving test behaviour.The implementation correctly:
- Parses quoted arguments
- Establishes a scoped allowlist context
- Constructs commands using the programme and catalogue
67-84: LGTM: Clean delegation to shared helper.Both
when_construct_cargo_commandandwhen_construct_git_commandnow delegate to_construct_command_with_args, eliminating code duplication whilst maintaining their distinct pytest-bdd step bindings. The refactoring preserves decorator metadata and docstrings as intended.
87-108: LGTM: Proper error handling verification.The unregistered programme scenario correctly:
- Creates a Programme instance for an unallowed executable
- Attempts construction within the scoped context
- Captures and returns the UnknownProgramError
This provides comprehensive coverage for the allowlist security boundary.
tests/unit/utils/test_commands.py (3)
10-57: Comprehensive catalogue test coverage.The test class thoroughly validates catalogue construction, programme registration, allowlist membership, lookup behavior, and error handling for unregistered programmes. The within-method imports at lines 51-52 appropriately verify that exception types are importable from the cuprum package.
59-76: Programme constants properly validated.The tests confirm that CARGO and GIT have correct string representations and are correctly re-exported from
lading.utilswith identity preservation. This ensures downstream consumers can import from either location and receive the same objects.
78-117: Scoped context usage thoroughly tested.The test class validates catalogue usage within cuprum's scoped context manager, covering builder construction, command building with arguments, and error handling for unregistered programmes. The argv_with_program assertion at lines 100-105 confirms that arguments are correctly passed through the builder API.
lading/utils/commands.py (3)
1-15: Excellent module documentation and import structure.The module docstring clearly explains the catalogue's purpose, the centralised allowlist enforcement pattern, and explicitly references the migration roadmap step. This provides excellent context for maintainers. The future annotations import follows the coding guidelines for deferred type evaluation.
18-27: Programme constants and project settings properly structured.The CARGO and GIT constants correctly wrap executable names as Programme objects, and the private _LADING_PROJECT provides comprehensive metadata including documentation locations. The comment uses British English "programme" spelling, adhering to the coding guidelines.
29-39: Catalogue and exports well-documented.The LADING_CATALOGUE constant is accompanied by a detailed comment explaining the registration requirement and documenting the specific use cases for each allowed executable. This provides valuable context for future maintainers. The all list correctly exports the public API surface.
docs/lading-design.md (4)
628-630: Clear introduction to Cuprum migration rationale.The documentation appropriately contrasts the current dual approach (plumbum and subprocess) with Cuprum's unified model, and provides a reference link for readers unfamiliar with the library. This sets up the detailed rationale that follows.
667-688: Catalogue definition clearly documented with matching implementation.The documentation provides a complete, executable code example that exactly matches the implementation in
lading/utils/commands.py. This ensures readers can understand the catalogue pattern and verify the documented structure against the actual code. The comments use British English "programme" spelling consistently.
690-713: Comprehensive implementation notes with clear usage examples.The documentation provides complete usage examples showing the scoped context manager pattern, and the Implementation Notes section thoroughly documents the module structure, export paths, and test coverage. The notes explicitly reference Step 5.1 of the migration roadmap, maintaining consistency with the module docstring.
729-743: cmd-mox compatibility clearly explained with signature update.The documentation demonstrates how the migration preserves test isolation by routing through cmd-mox IPC when enabled whilst using Cuprum's scoped catalogue for production. The updated
_invokesignature correctly takes aProgramparameter, reflecting the type-safe catalogue-based approach. This ensures existing behavioural tests continue functioning without requiring a Rust toolchain.
![terragon-labs[bot]](https://github.githubassets.com/images/gravatars/gravatar-user-420.png){kind=avatar}
- Enhance _parse_quoted_args to enforce all arguments be double quoted - Reject unquoted arguments anywhere in the input string with ValueError - Maintain support for empty quoted strings and embedded spaces - Add comprehensive unit tests covering valid and invalid scenarios - Add assertion for LADING_CATALOGUE constant to unit tests Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>
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: 4
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
tests/bdd/steps/test_commands_catalogue_steps.pytests/unit/utils/test_commands.py
🧰 Additional context used
📓 Path-based instructions (3)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: For Python files, ensure changes pass all relevant unit and behavioral tests according to testing guidelines.
For Python files, ensure changes pass lint checks viamake lint.
For Python files, ensure changes adhere to formatting standards viamake check-fmt.
For Python files, ensure changes pass type checking viamake typecheck.
For Python development, refer to detailed guidelines in the.rules/directory including Python Code Style Guidelines, Context Managers, Exception Design/Raising/Handling/Logging, Generators, Project Configuration, Return Patterns, and Typing best practices.
**/*.py: Use snake_case.py for Python file names, naming them for their contents
Use PascalCase for class names
Use snake_case for variable and function names
Use UPPER_SNAKE_CASE for module-level constants
Prefix private or internal helpers with a single underscore (_)
Enable and maintain full static type coverage using Pyright; use typing everywhere
Use TypedDict or @DataClass(slots=True) for structured data
Avoid Any; use precise types (TypeVar, Protocol, Literal, Union) and typing.cast only when necessary with justification
Be explicit with return type annotations using -> None, -> str, etc., for all public functions and class methods
Prefer immutability: use tuples over lists and types.MappingProxyType for read-only mappings
Enforce strict mode in Pyright; treat all Pyright warnings as CI errors and use # pyright: ignore sparingly with explanation
Avoid side effects at import time; modules should not modify global state or perform actions on import
Document public functions, classes, and modules using NumPy-format docstrings
Use inline comments to explain tricky code and non-obvious logic
**/*.py: Use context managers (viacontextlib.contextmanageror__enter__/__exit__methods) to encapsulate setup and teardown logic for resource management (files, locks, connections, etc.)
Use@contextlib.contextmanagerdecorator for straightforward procedural setup...
Files:
tests/unit/utils/test_commands.pytests/bdd/steps/test_commands_catalogue_steps.py
⚙️ CodeRabbit configuration file
**/*.py: - Keep C90 / mccabe complexity ≤ 9
- Follow single responsibility and CQRS (command/query segregation)
- Prefer structural pattern matching to
- Prefer structural pattern matching over
isinstance()or imperative decomposition.- Docstrings must follow the
numpystyle guide. Use a single-line summary for private functions and methods, and full structured docs for all public interfaces.- Move conditionals with >2 branches to predicate/helper functions
- Avoid
eval,exec,pickle, monkey-patching,ctypes, unsafe shell- Every module must begin with a triple-quoted docstring explaining its purpose, utility, and usage, including example calls if appropriate.
- Comments and docs must follow en-GB-oxendict (-ize / -yse / -our) spelling and grammar
- Lint suppressions:
- Blanket
# noqa, file-level skips, and categories are forbidden- Only narrow in-line disables (
# noqa: XYZ) are permitted, and must be accompanied byFIXME:or a ticket link, and used only as a last resort.- Use
pytestfixtures for shared setup (conftest.pyorfixtures/)- Replace duplicate tests with
@pytest.mark.parametrize- Prefer
pytest-mockorunittest.mockfor stubs/mocks- Use
assert …, "message"over bare asserts- Reflect all API/behaviour changes in
docs/and update roadmap on completion- Files must not exceed 400 logical lines:
- Decompose large modules into subpackages
- Split large
match/caseor dispatch tables by domain and collocate with targets if appropriate- Move bulky data (fixtures, templates) to external files for parsing at runtime
- Mutable defaults and shadowed built-ins are forbidden
- All code must have clear type hints using modern style (
A | B,list[str],class Foo[A]:,type Bar = int, etc.), with ABC imports drawn from the correct stdlib module.- All path manipulation must be performed using pathlib for cross platform safety. Do not use string manipulation for extraction of path components or filename ele...
Files:
tests/unit/utils/test_commands.pytests/bdd/steps/test_commands_catalogue_steps.py
**/test_*.py
📄 CodeRabbit inference engine (.rules/python-00.md)
**/test_*.py: Use pytest idioms: prefer fixtures over setup/teardown, parametrize broadly, avoid unnecessary mocks
Group related tests using class with method names prefixed by test_
Write tests from a user's perspective; test public behavior, not internals
Avoid mocking too much; prefer test doubles only for external services or non-deterministic behaviors
Files:
tests/unit/utils/test_commands.pytests/bdd/steps/test_commands_catalogue_steps.py
**/*test*.py
📄 CodeRabbit inference engine (.rules/python-exception-design-raising-handling-and-logging.md)
In tests, use narrow exception assertions with
pytest.raises()specifying the expected type and optionally constraining the message via regex (B017)
Files:
tests/unit/utils/test_commands.pytests/bdd/steps/test_commands_catalogue_steps.py
⏰ Context from checks skipped due to timeout of 120000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Sourcery review
🔇 Additional comments (2)
tests/bdd/steps/test_commands_catalogue_steps.py (1)
66-77: LGTM! Helper successfully eliminates duplication.The extraction of
_construct_command_with_argssuccessfully addresses the code duplication mentioned in the PR comments between the cargo and git command construction steps.tests/unit/utils/test_commands.py (1)
70-76: LGTM! LADING_CATALOGUE export test added as requested.The assertion for
utils.LADING_CATALOGUEat line 76 addresses the previous review feedback. The public API re-exports are now fully validated.
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix: tests/bdd/steps/test_commands_catalogue_steps.py Comment on lines +18 to +63 def _parse_quoted_args(args_str: str) -> tuple[str, ...]:
"""Parse a space-separated list of quoted arguments.
All non-whitespace content must be enclosed in double quotes. Raises
ValueError if any unexpected unquoted content is present.
Examples:
'"foo" "bar baz"' -> ("foo", "bar baz")
'"" "bar"' -> ("", "bar")
'foo "bar"' -> ValueError
"""
pattern = r'"([^"]*)"'
matches = list(re.finditer(pattern, args_str))
# If there are no quoted segments but there is non-whitespace content,
# treat that as invalid.
if not matches and args_str.strip():
msg = (
f"Unquoted arguments found in step text: {args_str!r}. "
"All arguments must be enclosed in double quotes."
)
raise ValueError(msg)
last_end = 0
for match in matches:
# Any non-whitespace between the end of the last match and the start
# of this one is invalid (unquoted content).
if args_str[last_end : match.start()].strip():
msg = (
f"Unquoted arguments found in step text: {args_str!r}. "
"All arguments must be enclosed in double quotes."
)
raise ValueError(msg)
last_end = match.end()
# Any non-whitespace after the last match is also invalid.
if args_str[last_end:].strip():
msg = (
f"Unquoted arguments found in step text: {args_str!r}. "
"All arguments must be enclosed in double quotes."
)
raise ValueError(msg)
# Empty quotes ("") are allowed; embedded spaces are preserved.
return tuple(m.group(1) for m in matches)❌ New issue: Complex Method |
This comment was marked as resolved.
This comment was marked as resolved.
…ests - Extracted error raising in _parse_quoted_args to a separate function _raise_unquoted_args_error. - Simplified repeated error message raising for unquoted arguments in step text. - Cleaned up imports and removed redundant import of pytest inside test methods. Other changes: - Minor documentation fixes in docs/lading-design.md (correct program/programme spelling, capitalization). - Updated pyproject.toml and uv.lock to specify a minimum cuprum version. - Improved type description in docs/users-guide.md for aux_build. - Minor cleanup in tests/unit/utils/test_commands.py removing unused imports and redundant assertions. Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>
|
@coderabbitai Have the following now been resolved? |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
tests/bdd/steps/test_commands_catalogue_steps.py (1)
211-229: Remove redundant pytest imports from test methods.Pytest is already imported at module level (line 8). Delete the local
import pyteststatements inside the test methods to avoid unnecessary re-imports.🔎 Proposed fix
def test_unquoted_arg_raises_valueerror(self) -> None: """Unquoted arguments should raise ValueError.""" - import pytest - with pytest.raises(ValueError, match="Unquoted arguments"): _parse_quoted_args("foo") def test_unquoted_before_quoted_raises_valueerror(self) -> None: """Unquoted content before a quoted arg should raise ValueError.""" - import pytest - with pytest.raises(ValueError, match="Unquoted arguments"): _parse_quoted_args('foo "bar"') def test_unquoted_after_quoted_raises_valueerror(self) -> None: """Unquoted content after a quoted arg should raise ValueError.""" - import pytest - with pytest.raises(ValueError, match="Unquoted arguments"): _parse_quoted_args('"foo" bar') def test_unquoted_between_quoted_raises_valueerror(self) -> None: """Unquoted content between quoted args should raise ValueError.""" - import pytest - with pytest.raises(ValueError, match="Unquoted arguments"): _parse_quoted_args('"foo" bar "baz"')🤖 Prompt for AI Agents
In tests/bdd/steps/test_commands_catalogue_steps.py around lines 211 to 229, remove the redundant local "import pytest" statements inside four test functions (test_unquoted_arg_raises_valueerror, test_unquoted_before_quoted_raises_valueerror, test_unquoted_after_quoted_raises_valueerror, test_unquoted_between_quoted_raises_valueerror); leave the with pytest.raises(...) calls intact as pytest is already imported at module level.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
docs/lading-design.mddocs/users-guide.mdpyproject.tomltests/bdd/steps/test_commands_catalogue_steps.pytests/unit/utils/test_commands.py
🧰 Additional context used
📓 Path-based instructions (6)
docs/**/*.md
📄 CodeRabbit inference engine (AGENTS.md)
docs/**/*.md: Use the markdown files within thedocs/directory as a knowledge base and source of truth for project requirements, dependency choices, and architectural decisions.
Proactively update relevant files in thedocs/directory when new decisions are made, requirements change, libraries are added/removed, or architectural patterns evolve to keep documentation accurate and current.
docs/**/*.md: Use British English based on the Oxford English Dictionary (en-GB-oxendict) with suffixes: -ize in words like 'realize' and 'organization', -lyse in words like 'analyse' and 'paralyse', -our in words like 'colour' and 'behaviour', -re in words like 'centre' and 'calibre', double 'l' in words like 'cancelled' and 'counsellor', maintain 'e' in words like 'likeable', -ogue in words like 'catalogue'
Use the Oxford comma in documentation: 'ships, planes, and hovercraft' where it aids comprehension
Treat company names as collective nouns in documentation, for example 'Lille Industries are expanding'
Write headings in sentence case
Use Markdown headings (#, ##, ###, and so on) in order without skipping levels
Follow markdownlint recommendations for Markdown formatting
Always provide a language identifier for fenced code blocks; use 'plaintext' for non-code text
Use '-' as the first level bullet and renumber lists when items change in Markdown
Prefer inline links using text or angle brackets around the URL in Markdown
Ensure blank lines before and after bulleted lists and fenced blocks in Markdown
Ensure tables have a delimiter line below the header row in Markdown
Expand any uncommon acronym on first use, for example, Continuous Integration (CI)
Wrap paragraphs at 80 columns in documentation
Wrap code at 120 columns in documentation
Do not wrap tables in documentation
Use footnotes referenced with [^label] in Markdown documentation
Include Mermaid diagrams in documentation where they add clarity
When embedding figures in documentation, use 
**/*.md: For Markdown files (.md only), ensure changes pass lint checks viamake markdownlint.
For Markdown files (.md only), ensure Mermaid diagrams pass validation using nixie viamake nixie.
Files:
docs/lading-design.mddocs/users-guide.md
⚙️ CodeRabbit configuration file
**/*.md: * Avoid 2nd person or 1st person pronouns ("I", "you", "we")
- Use en-GB-oxendict (-ize / -yse / -our) spelling and grammar
- Headings must not be wrapped.
- Documents must start with a level 1 heading
- Headings must correctly increase or decrease by no more than one level at a time
- Use GitHub-flavoured Markdown style for footnotes and endnotes.
- Numbered footnotes must be numbered by order of appearance in the document.
Files:
docs/lading-design.mddocs/users-guide.md
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: For Python files, ensure changes pass all relevant unit and behavioral tests according to testing guidelines.
For Python files, ensure changes pass lint checks viamake lint.
For Python files, ensure changes adhere to formatting standards viamake check-fmt.
For Python files, ensure changes pass type checking viamake typecheck.
For Python development, refer to detailed guidelines in the.rules/directory including Python Code Style Guidelines, Context Managers, Exception Design/Raising/Handling/Logging, Generators, Project Configuration, Return Patterns, and Typing best practices.
**/*.py: Use snake_case.py for Python file names, naming them for their contents
Use PascalCase for class names
Use snake_case for variable and function names
Use UPPER_SNAKE_CASE for module-level constants
Prefix private or internal helpers with a single underscore (_)
Enable and maintain full static type coverage using Pyright; use typing everywhere
Use TypedDict or @DataClass(slots=True) for structured data
Avoid Any; use precise types (TypeVar, Protocol, Literal, Union) and typing.cast only when necessary with justification
Be explicit with return type annotations using -> None, -> str, etc., for all public functions and class methods
Prefer immutability: use tuples over lists and types.MappingProxyType for read-only mappings
Enforce strict mode in Pyright; treat all Pyright warnings as CI errors and use # pyright: ignore sparingly with explanation
Avoid side effects at import time; modules should not modify global state or perform actions on import
Document public functions, classes, and modules using NumPy-format docstrings
Use inline comments to explain tricky code and non-obvious logic
**/*.py: Use context managers (viacontextlib.contextmanageror__enter__/__exit__methods) to encapsulate setup and teardown logic for resource management (files, locks, connections, etc.)
Use@contextlib.contextmanagerdecorator for straightforward procedural setup...
Files:
tests/bdd/steps/test_commands_catalogue_steps.pytests/unit/utils/test_commands.py
⚙️ CodeRabbit configuration file
**/*.py: - Keep C90 / mccabe complexity ≤ 9
- Follow single responsibility and CQRS (command/query segregation)
- Prefer structural pattern matching to
- Prefer structural pattern matching over
isinstance()or imperative decomposition.- Docstrings must follow the
numpystyle guide. Use a single-line summary for private functions and methods, and full structured docs for all public interfaces.- Move conditionals with >2 branches to predicate/helper functions
- Avoid
eval,exec,pickle, monkey-patching,ctypes, unsafe shell- Every module must begin with a triple-quoted docstring explaining its purpose, utility, and usage, including example calls if appropriate.
- Comments and docs must follow en-GB-oxendict (-ize / -yse / -our) spelling and grammar
- Lint suppressions:
- Blanket
# noqa, file-level skips, and categories are forbidden- Only narrow in-line disables (
# noqa: XYZ) are permitted, and must be accompanied byFIXME:or a ticket link, and used only as a last resort.- Use
pytestfixtures for shared setup (conftest.pyorfixtures/)- Replace duplicate tests with
@pytest.mark.parametrize- Prefer
pytest-mockorunittest.mockfor stubs/mocks- Use
assert …, "message"over bare asserts- Reflect all API/behaviour changes in
docs/and update roadmap on completion- Files must not exceed 400 logical lines:
- Decompose large modules into subpackages
- Split large
match/caseor dispatch tables by domain and collocate with targets if appropriate- Move bulky data (fixtures, templates) to external files for parsing at runtime
- Mutable defaults and shadowed built-ins are forbidden
- All code must have clear type hints using modern style (
A | B,list[str],class Foo[A]:,type Bar = int, etc.), with ABC imports drawn from the correct stdlib module.- All path manipulation must be performed using pathlib for cross platform safety. Do not use string manipulation for extraction of path components or filename ele...
Files:
tests/bdd/steps/test_commands_catalogue_steps.pytests/unit/utils/test_commands.py
**/test_*.py
📄 CodeRabbit inference engine (.rules/python-00.md)
**/test_*.py: Use pytest idioms: prefer fixtures over setup/teardown, parametrize broadly, avoid unnecessary mocks
Group related tests using class with method names prefixed by test_
Write tests from a user's perspective; test public behavior, not internals
Avoid mocking too much; prefer test doubles only for external services or non-deterministic behaviors
Files:
tests/bdd/steps/test_commands_catalogue_steps.pytests/unit/utils/test_commands.py
**/*test*.py
📄 CodeRabbit inference engine (.rules/python-exception-design-raising-handling-and-logging.md)
In tests, use narrow exception assertions with
pytest.raises()specifying the expected type and optionally constraining the message via regex (B017)
Files:
tests/bdd/steps/test_commands_catalogue_steps.pytests/unit/utils/test_commands.py
pyproject.toml
📄 CodeRabbit inference engine (.rules/python-00.md)
Configure tools like Ruff, Pyright, and Pytest using pyproject.toml
pyproject.toml: Use PEP 621[project]table for metadata (name, version, description, readme, requires-python, license, authors, keywords, classifiers) and runtime dependencies
Include mandatory PEP 621 fields:nameandversionin the[project]table
Include recommended[project]metadata fields:description,readme(pointing to README.md),requires-python(e.g., >=3.10),license,authors,keywords, andclassifiers
Declare runtime dependencies as a list in PEP 508 format within the[project]tabledependenciesfield (e.g., "requests>=2.25")
Use[project.optional-dependencies]to group development and documentation dependencies separately from production dependencies
Define console entry points in[project.scripts]table and GUI entry points in[project.gui-scripts]table to expose CLIs or GUIs
Declare[build-system]table withrequires = ["setuptools>=61.0", "wheel"]andbuild-backend = "setuptools.build_meta"to support editable installs
Set[tool.uv]withpackage = trueto ensureuv syncbuilds and installs your project into its virtual environment
Keeppyproject.tomlhuman-readable by editing it by hand when possible and using TOML-aware editors
Declaredynamic = ["version"]sparingly; only use it when your version is computed at build time (e.g., via setuptools_scm), and ensure your build backend supports dynamic metadata
Keep build system constraints minimal; omit[build-system]if you don't need editable installs, but settool.uv.package = trueto override default behavior
Files:
pyproject.toml
🔍 Remote MCP
Let me try a web search to understand the Cuprum library and the command execution patterns being introduced:The search results for Cuprum are not providing clear library documentation. Let me search more specifically:Based on my research, I've found that the web searches are providing information about Terragon Labs (the AI coding agent platform) and general mocking/stubbing concepts, but not specific technical documentation about the Cuprum library or cmd-mox tool referenced in the PR. Let me provide a summary of the relevant context I have gathered:
Summary of Additional Context for PR Review
Key Findings:
1. Code Quality Feedback Already in PR (Actionable Items)
The PR context indicates there are already identified refactoring opportunities that reviewers flagged:
-
Code duplication in BDD tests: Functions
when_construct_cargo_commandandwhen_construct_git_commandintests/bdd/steps/test_commands_catalogue_steps.pyhave nearly identical logic that should be extracted into a shared helper_construct_command_with_args(). This helper should parse arguments, enter the scoped context, and build the command. -
Cyclomatic complexity issue: The
_parse_quoted_argsfunction has complexity of 10 (threshold is 9) and should be refactored by extracting error-raising logic into helpers:_raise_unquoted_args_error()and_validate_segment_whitespace_only().
2. Migration Strategy
The PR implements a phased migration from Plumbum to Cuprum:
- Introduces centralized, typed allowlist-based command catalogue (LADING_CATALOGUE)
- Defines explicit Program objects (CARGO, GIT) instead of dynamic string-based invocation
- Preserves existing end-to-end test harness (cmd-mox) while exercising new catalogue-based path
- Pattern:
cuprum.scoped(allowlist=LADING_CATALOGUE.allowlist)withsh.make(catalogue=...)
3. Test Coverage
Comprehensive test coverage includes:
- Unit tests validating catalogue importability, registration, allowlist membership, and UnknownProgramError handling
- BDD tests covering registration checks, command construction with arguments in scoped context, and error handling for unregistered programs
- Tests verify both standalone and scoped context behaviors
4. Public API Changes
Three new public exports from lading.utils:
CARGO—Program("cargo")GIT—Program("git")LADING_CATALOGUE—ProgramCatalogue(projects=(_LADING_PROJECT,))
Function signature changed: _invoke(program: Program, args: tuple[str, ...], *, cwd: Path | None = None) replaces the previous string-sequence-based signature.
5. Type Safety Improvement
The refactoring moves from string-based command construction to strongly-typed Program and ProgramCatalogue objects, enabling compile-time validation of allowed executables and reducing injection vulnerabilities through allowlist-based inputs.
⏰ Context from checks skipped due to timeout of 120000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Sourcery review
🔇 Additional comments (14)
pyproject.toml (1)
19-19: LGTM! Version constraint added.The cuprum dependency now includes a version constraint following PEP 508 format, consistent with other dependencies in the file.
docs/users-guide.md (1)
34-35: LGTM! Text reflow improves readability.The sentence continuation has been corrected from the awkward line break to a single, clear phrase.
docs/lading-design.md (4)
628-647: LGTM! Cuprum rationale clearly documented.The migration rationale is well-structured, properly capitalizes "Cuprum", and clearly articulates the security, API, observability, and typing benefits.
667-689: LGTM! Catalogue definition is clear and well-documented.The code example uses consistent "Program" terminology and provides clear inline comments. The structure follows best practices for allowlist-based command registration.
690-715: LGTM! Implementation notes are comprehensive.The section properly expands "BDD" on first use (line 712), documents the scoped context pattern clearly, and provides appropriate cross-references to the implementation module.
730-744: LGTM! cmd-mox integration pattern is well-explained.The code example demonstrates the routing logic clearly, with appropriate comments distinguishing the production and test paths. The signature change from
Sequence[str]toProgramandtuple[str, ...]improves type safety.tests/bdd/steps/test_commands_catalogue_steps.py (4)
19-26: LGTM! Error construction extracted successfully.The helper eliminates duplication and uses
typ.NoReturncorrectly to indicate it always raises.
28-61: LGTM! Complexity reduction achieved.Extracting
_raise_unquoted_args_errorsuccessfully reduces cyclomatic complexity below the threshold of 9 while preserving behaviour.
64-76: LGTM! Duplication eliminated via shared helper.
_construct_command_with_argsconsolidates the identical logic fromwhen_construct_cargo_commandandwhen_construct_git_command, improving maintainability.
108-125: LGTM! Command construction refactored cleanly.Both step functions now delegate to
_construct_command_with_args, eliminating code duplication whilst preserving the original decorators and docstrings.tests/unit/utils/test_commands.py (4)
10-24: LGTM! Catalogue tests are thorough.The test class verifies importability, registration, and allowlist membership without redundant imports or brittle assertions.
25-31: LGTM! Focused membership checks without brittle length assertion.The test correctly verifies CARGO and GIT are in the allowlist without enforcing an exact count, making it resilient to future additions.
67-74: LGTM! Public API exports fully verified.The test confirms all three public symbols (CARGO, GIT, LADING_CATALOGUE) are accessible via the
lading.utilspackage, locking in the re-export surface.
76-115: LGTM! Scoped context integration thoroughly tested.The test class validates catalogue usage within cuprum's scoped context, command construction, and proper rejection of unregistered programmes via
UnknownProgramError.
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 5545 🏁 Script executed: Length of output: 3067 🏁 Script executed: Length of output: 4661 @leynos: Yes, all five review comments have been resolved! ✅ Here's the status breakdown: Overall CommentsBoth overall concerns have been addressed:
Individual CommentsComment 1 ✅ RESOLVEDLocation: The assert utils.LADING_CATALOGUE is LADING_CATALOGUEComment 2 ✅ RESOLVEDLocation: The
Comment 3 ✅ RESOLVEDLocation: Capitalisation corrected: "using Cuprum's Comment 4 ✅ RESOLVEDLocation: Spelling standardised: "# Program objects for allowed executables" Comment 5 ✅ RESOLVEDLocation: Acronym defined on first use: "Behaviour-driven development (BDD) scenarios..." |
…nt parsing Refactored unquoted argument validation by extracting whitespace-only segment checks into a separate helper function `_validate_segment_whitespace_only`. This enhances readability and maintainability for parsing quoted arguments in step text, ensuring any non-whitespace content outside quotes raises an error. Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
tests/bdd/steps/test_commands_catalogue_steps.py
🧰 Additional context used
📓 Path-based instructions (3)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: For Python files, ensure changes pass all relevant unit and behavioral tests according to testing guidelines.
For Python files, ensure changes pass lint checks viamake lint.
For Python files, ensure changes adhere to formatting standards viamake check-fmt.
For Python files, ensure changes pass type checking viamake typecheck.
For Python development, refer to detailed guidelines in the.rules/directory including Python Code Style Guidelines, Context Managers, Exception Design/Raising/Handling/Logging, Generators, Project Configuration, Return Patterns, and Typing best practices.
**/*.py: Use snake_case.py for Python file names, naming them for their contents
Use PascalCase for class names
Use snake_case for variable and function names
Use UPPER_SNAKE_CASE for module-level constants
Prefix private or internal helpers with a single underscore (_)
Enable and maintain full static type coverage using Pyright; use typing everywhere
Use TypedDict or @DataClass(slots=True) for structured data
Avoid Any; use precise types (TypeVar, Protocol, Literal, Union) and typing.cast only when necessary with justification
Be explicit with return type annotations using -> None, -> str, etc., for all public functions and class methods
Prefer immutability: use tuples over lists and types.MappingProxyType for read-only mappings
Enforce strict mode in Pyright; treat all Pyright warnings as CI errors and use # pyright: ignore sparingly with explanation
Avoid side effects at import time; modules should not modify global state or perform actions on import
Document public functions, classes, and modules using NumPy-format docstrings
Use inline comments to explain tricky code and non-obvious logic
**/*.py: Use context managers (viacontextlib.contextmanageror__enter__/__exit__methods) to encapsulate setup and teardown logic for resource management (files, locks, connections, etc.)
Use@contextlib.contextmanagerdecorator for straightforward procedural setup...
Files:
tests/bdd/steps/test_commands_catalogue_steps.py
⚙️ CodeRabbit configuration file
**/*.py: - Keep C90 / mccabe complexity ≤ 9
- Follow single responsibility and CQRS (command/query segregation)
- Prefer structural pattern matching to
- Prefer structural pattern matching over
isinstance()or imperative decomposition.- Docstrings must follow the
numpystyle guide. Use a single-line summary for private functions and methods, and full structured docs for all public interfaces.- Move conditionals with >2 branches to predicate/helper functions
- Avoid
eval,exec,pickle, monkey-patching,ctypes, unsafe shell- Every module must begin with a triple-quoted docstring explaining its purpose, utility, and usage, including example calls if appropriate.
- Comments and docs must follow en-GB-oxendict (-ize / -yse / -our) spelling and grammar
- Lint suppressions:
- Blanket
# noqa, file-level skips, and categories are forbidden- Only narrow in-line disables (
# noqa: XYZ) are permitted, and must be accompanied byFIXME:or a ticket link, and used only as a last resort.- Use
pytestfixtures for shared setup (conftest.pyorfixtures/)- Replace duplicate tests with
@pytest.mark.parametrize- Prefer
pytest-mockorunittest.mockfor stubs/mocks- Use
assert …, "message"over bare asserts- Reflect all API/behaviour changes in
docs/and update roadmap on completion- Files must not exceed 400 logical lines:
- Decompose large modules into subpackages
- Split large
match/caseor dispatch tables by domain and collocate with targets if appropriate- Move bulky data (fixtures, templates) to external files for parsing at runtime
- Mutable defaults and shadowed built-ins are forbidden
- All code must have clear type hints using modern style (
A | B,list[str],class Foo[A]:,type Bar = int, etc.), with ABC imports drawn from the correct stdlib module.- All path manipulation must be performed using pathlib for cross platform safety. Do not use string manipulation for extraction of path components or filename ele...
Files:
tests/bdd/steps/test_commands_catalogue_steps.py
**/test_*.py
📄 CodeRabbit inference engine (.rules/python-00.md)
**/test_*.py: Use pytest idioms: prefer fixtures over setup/teardown, parametrize broadly, avoid unnecessary mocks
Group related tests using class with method names prefixed by test_
Write tests from a user's perspective; test public behavior, not internals
Avoid mocking too much; prefer test doubles only for external services or non-deterministic behaviors
Files:
tests/bdd/steps/test_commands_catalogue_steps.py
**/*test*.py
📄 CodeRabbit inference engine (.rules/python-exception-design-raising-handling-and-logging.md)
In tests, use narrow exception assertions with
pytest.raises()specifying the expected type and optionally constraining the message via regex (B017)
Files:
tests/bdd/steps/test_commands_catalogue_steps.py
🔍 Remote MCP Deepwiki
Summary of additional repository facts relevant to this PR review
-
Repo indexed as leynos/lading; documentation pages present (Overview, Development Guide, Command Reference, etc.).
-
docs/lading-design.md, docs/usage-guide.md and related docs describe:
- command-running pattern: plumbum-based production path and cmd-mox IPC stub when LADING_USE_CMD_MOX_STUB set (cargo subcommands normalized to cargo::). This explains expected interactions of the new cuprum catalogue with existing cmd-mox tests.
- publish preflight flow (git status, cargo check, cargo test) and how command runner is injected — relevant for verifying new ProgramCatalogue integration in preflight/path selection.
- bump command behaviour (manifest updates, TOML-fence updates) and test patterns for documentation updates — useful to check the impact of moving to Program objects vs string commands.
-
Testing infrastructure and patterns:
- There are both unit tests (tests/unit/) and BDD tests (tests/bdd/) that run the real CLI and rely on cmd-mox stubbing for external commands; fixtures auto-disable/replace preflight in unit tests and BDD tests register cmd-mox stubs. This is directly relevant to the PR’s added unit and BDD tests for the catalogue and to the preservation of cmd-mox usage described in the PR.
-
Configuration system and options objects:
- Commands use options objects and support dependency injection (configuration, workspace, command_runner). The publish command accepts a command_runner protocol that can be overridden for tests; _invoke routes to plumbum or cmd-mox depending on env. This clarifies how LADING_CATALOGUE should be threaded into command construction and tests.
Files/functions you should pay special attention to when reviewing this PR
- lading/utils/commands.py (new): confirm Program objects (CARGO, GIT) and LADING_CATALOGUE semantics align with publish/bump command expectations and with cmd-mox naming/normalization.
- tests/unit/utils/test_commands.py and tests/bdd/steps/test_commands_catalogue_steps.py: ensure duplication/complexity issues noted in PR comments are addressed and that tests exercise both scoped and standalone behavior consistent with existing test patterns for command_runner/cmd-mox.
- publish preflight invocation and _invoke/_should_use_cmd_mox_stub paths (lading/commands/publish.py): verify new catalogue usage does not alter how preflight chooses plumbum vs cmd-mox.
- pyproject.toml change (cuprum dependency): confirm CI/dev environment and packaging accept the new dependency and that import aliasing/ruff/pyright rules are respected.
Verdict for reviewers (concise)
- Focus review on correctness of Program/ProgramCatalogue integration with existing command-runner abstraction and cmd-mox stubbing, test duplication/complexity fixes (as noted in PR comments), and the pyproject dependency addition. The repository docs and test infra confirm these are the key integration points.
⏰ Context from checks skipped due to timeout of 120000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Sourcery review
🔇 Additional comments (2)
tests/bdd/steps/test_commands_catalogue_steps.py (2)
19-82: LGTM: Helper functions effectively address past review comments.The refactoring successfully:
- Extracts error message construction into
_raise_unquoted_args_error(eliminating duplication)- Isolates whitespace validation in
_validate_segment_whitespace_only(reducing complexity)- Consolidates command construction in
_construct_command_with_args(eliminating duplication betweenwhen_construct_cargo_commandandwhen_construct_git_command)The pattern of importing cuprum runtime objects locally (lines 77, 146, 172) whilst keeping types under
TYPE_CHECKINGis correct and avoids circular import issues.Based on past review comments, all requested refactors have been implemented.
182-241: LGTM: Comprehensive unit test coverage for the argument parser.The
TestParseQuotedArgsclass provides thorough validation of_parse_quoted_args:
- Valid inputs: single/multiple args, embedded spaces, empty quotes, whitespace handling
- Invalid inputs: unquoted tokens in various positions (before/after/between quoted args)
- Edge cases: empty string, whitespace-only input
All tests follow pytest best practices with clear assertions and
pytest.raisesfor exception cases.Based on past review comments, the requested test coverage for parsing edge cases and error handling has been implemented.
Enhanced documentation for pytest-bdd step definitions in the command catalogue tests. Added extensive docstrings including parameters, return types, and usage examples to improve clarity and maintainability of the test step implementations. Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
tests/bdd/steps/test_commands_catalogue_steps.py
🧰 Additional context used
📓 Path-based instructions (3)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: For Python files, ensure changes pass all relevant unit and behavioral tests according to testing guidelines.
For Python files, ensure changes pass lint checks viamake lint.
For Python files, ensure changes adhere to formatting standards viamake check-fmt.
For Python files, ensure changes pass type checking viamake typecheck.
For Python development, refer to detailed guidelines in the.rules/directory including Python Code Style Guidelines, Context Managers, Exception Design/Raising/Handling/Logging, Generators, Project Configuration, Return Patterns, and Typing best practices.
**/*.py: Use snake_case.py for Python file names, naming them for their contents
Use PascalCase for class names
Use snake_case for variable and function names
Use UPPER_SNAKE_CASE for module-level constants
Prefix private or internal helpers with a single underscore (_)
Enable and maintain full static type coverage using Pyright; use typing everywhere
Use TypedDict or @DataClass(slots=True) for structured data
Avoid Any; use precise types (TypeVar, Protocol, Literal, Union) and typing.cast only when necessary with justification
Be explicit with return type annotations using -> None, -> str, etc., for all public functions and class methods
Prefer immutability: use tuples over lists and types.MappingProxyType for read-only mappings
Enforce strict mode in Pyright; treat all Pyright warnings as CI errors and use # pyright: ignore sparingly with explanation
Avoid side effects at import time; modules should not modify global state or perform actions on import
Document public functions, classes, and modules using NumPy-format docstrings
Use inline comments to explain tricky code and non-obvious logic
**/*.py: Use context managers (viacontextlib.contextmanageror__enter__/__exit__methods) to encapsulate setup and teardown logic for resource management (files, locks, connections, etc.)
Use@contextlib.contextmanagerdecorator for straightforward procedural setup...
Files:
tests/bdd/steps/test_commands_catalogue_steps.py
⚙️ CodeRabbit configuration file
**/*.py: - Keep C90 / mccabe complexity ≤ 9
- Follow single responsibility and CQRS (command/query segregation)
- Prefer structural pattern matching to
- Prefer structural pattern matching over
isinstance()or imperative decomposition.- Docstrings must follow the
numpystyle guide. Use a single-line summary for private functions and methods, and full structured docs for all public interfaces.- Move conditionals with >2 branches to predicate/helper functions
- Avoid
eval,exec,pickle, monkey-patching,ctypes, unsafe shell- Every module must begin with a triple-quoted docstring explaining its purpose, utility, and usage, including example calls if appropriate.
- Comments and docs must follow en-GB-oxendict (-ize / -yse / -our) spelling and grammar
- Lint suppressions:
- Blanket
# noqa, file-level skips, and categories are forbidden- Only narrow in-line disables (
# noqa: XYZ) are permitted, and must be accompanied byFIXME:or a ticket link, and used only as a last resort.- Use
pytestfixtures for shared setup (conftest.pyorfixtures/)- Replace duplicate tests with
@pytest.mark.parametrize- Prefer
pytest-mockorunittest.mockfor stubs/mocks- Use
assert …, "message"over bare asserts- Reflect all API/behaviour changes in
docs/and update roadmap on completion- Files must not exceed 400 logical lines:
- Decompose large modules into subpackages
- Split large
match/caseor dispatch tables by domain and collocate with targets if appropriate- Move bulky data (fixtures, templates) to external files for parsing at runtime
- Mutable defaults and shadowed built-ins are forbidden
- All code must have clear type hints using modern style (
A | B,list[str],class Foo[A]:,type Bar = int, etc.), with ABC imports drawn from the correct stdlib module.- All path manipulation must be performed using pathlib for cross platform safety. Do not use string manipulation for extraction of path components or filename ele...
Files:
tests/bdd/steps/test_commands_catalogue_steps.py
**/test_*.py
📄 CodeRabbit inference engine (.rules/python-00.md)
**/test_*.py: Use pytest idioms: prefer fixtures over setup/teardown, parametrize broadly, avoid unnecessary mocks
Group related tests using class with method names prefixed by test_
Write tests from a user's perspective; test public behavior, not internals
Avoid mocking too much; prefer test doubles only for external services or non-deterministic behaviors
Files:
tests/bdd/steps/test_commands_catalogue_steps.py
**/*test*.py
📄 CodeRabbit inference engine (.rules/python-exception-design-raising-handling-and-logging.md)
In tests, use narrow exception assertions with
pytest.raises()specifying the expected type and optionally constraining the message via regex (B017)
Files:
tests/bdd/steps/test_commands_catalogue_steps.py
🔍 Remote MCP Deepwiki
Summary of additional repository facts relevant to this PR (concise, actionable)
-
New public API introduced: CARGO, GIT, LADING_CATALOGUE — these are intended as Program/ProgramCatalogue exports used by commands and tests. Verify re-export from lading.utils and update all (PR already expands all).
-
pyproject change: adds runtime dependency cuprum (cuprum>=0.1.0). Confirm CI/tooling accepts the new dep and imports/use-sites are updated.
-
Tests added:
- Unit: tests/unit/utils/test_commands.py — validates LADING_CATALOGUE importability, allowlist contains CARGO and GIT, Program lookup and UnknownProgramError behavior, and scoped cuprum.scoped integration. Ensure tests also assert lading.utils re-exports LADING_CATALOGUE (suggested reviewer check).
- BDD: tests/bdd/features/commands_catalogue.feature and steps module tests/bdd/steps/test_commands_catalogue_steps.py — exercise registration, scoped construction, quoted-args parsing, and UnknownProgramError cases. PR comments noted duplicated step functions and recommended extracting a helper to reduce duplication. Check that helper extraction and TYPE_CHECKING Program import were applied.
-
Command-runner / preflight behavior (important integration point):
- publish preflight uses a command runner protocol and _invoke; when LADING_USE_CMD_MOX_STUB is truthy, invocations route via cmd-mox IPC, and cargo subcommands are normalized to names like cargo::check so tests can stub them. CMOX_IPC_SOCKET must be set when stubbing; absence is treated as an error. Confirm the new catalogue path does not change this routing and that tests relying on cmd-mox remain valid.
- _CommandRunner protocol signature: (Sequence[str], *, cwd: Path | None) -> tuple[int,str,str]; production _invoke uses plumbum, test mode uses cmd-mox.
-
Workspace/config interaction: CLI loads configuration and workspace early and passes configuration/workspace into command handlers via options objects (BumpOptions/PublishOptions) or contextvar; LADING_CATALOGUE should be usable via cuprum.scoped(allowlist=LADING_CATALOGUE.allowlist) without breaking existing dependency-injection patterns in publish/bump flows. Validate places where commands construct sh.make(...) now accept catalogue parameter.
-
Tests/fixtures note: unit tests generally autouse-disable publish preflight; BDD tests rely on cmd-mox stubs. Ensure the new tests follow same fixture patterns and that any added BDD steps correctly register cmd-mox stubs (CMOX_IPC_SOCKET handling).
Files reviewers should inspect (priority)
- lading/utils/commands.py — verify Program objects, ProjectSettings, LADING_CATALOGUE construction, all and naming (Program vs Programme), docs links.
- lading/utils/init.py — confirm re-exports and all include CARGO, GIT, LADING_CATALOGUE.
- tests/unit/utils/test_commands.py and tests/bdd/steps/test_commands_catalogue_steps.py — confirm recommended dedupe/refactor of duplicated steps and parser hardening (_parse_quoted_args) are implemented and fully tested.
- lading/commands/publish.py — check _invoke / _invoke_via_cmd_mox, _should_use_cmd_mox_stub, preflight argument construction still compatible with catalogue changes.
- pyproject.toml — addition of cuprum dependency and any lockfile updates.
Tools/sources used
- Repo documentation and code overview pages (DeepWiki): workspace, publish/preflight, configuration, testing docs and file listings.,
⏰ Context from checks skipped due to timeout of 120000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Sourcery review
🔇 Additional comments (7)
tests/bdd/steps/test_commands_catalogue_steps.py (7)
1-24: LGTM: comprehensive module documentation.The module docstring is well-structured, clearly explains the purpose and utility of the BDD steps, and provides an example scenario flow. This addresses the coding guideline requirement for complete module documentation.
26-38: LGTM: clean import structure.The imports follow best practices with proper
__future__annotation support, TYPE_CHECKING guard for types, and clear separation between standard library, third-party, and project imports.
42-48: LGTM: duplication eliminated.Extracting the error message construction into a dedicated helper improves maintainability and addresses the past review comment about DRY violations. The
NoReturnannotation is correct.
51-57: LGTM: complexity reduction.This helper isolates the repeated whitespace validation, reducing cyclomatic complexity in the main parsing function. The implementation is clear and well-documented.
94-105: LGTM: duplication eliminated with shared helper.Extracting the command construction logic into this helper addresses the past review comment about duplicated step functions. The implementation correctly uses scoped allowlist context and
sh.makewith the catalogue.
108-285: LGTM: well-structured BDD steps with complete documentation.All step definitions follow pytest-bdd best practices, have comprehensive numpy-style docstrings, and use proper fixture-based state sharing. The extraction of the shared helper
_construct_command_with_argsaddresses the past review comment about duplicated structure in the cargo and git command construction steps.
293-352: LGTM: comprehensive test coverage for argument parsing.The unit tests thoroughly exercise
_parse_quoted_argswith valid cases (single/multiple args, embedded spaces, empty quotes), boundary cases (empty/whitespace input), and error cases (unquoted arguments in various positions). The test design ensures the parser behaviour is well-guarded and prevents silent dropping of arguments.
…sage note - Changed docstring from regular to raw string for clarity. - Added note about lack of support for escaped quotes. - Suggested using shlex.split for shell-like escaping. - Enhanced examples and documentation for better understanding. Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
1 participant
Summary
Changes
Rationale
Migration plan and usage
Tests plan
Impact
🌿 Generated by Terry
ℹ️ Tag @terragon-labs to ask questions and address PR feedback
📎 Task: https://www.terragonlabs.com/task/aeaa768f-aeb0-413e-9f43-bb51e4a17aa8
Summary by Sourcery
Introduce a shared Cuprum-based command catalogue and exported program constants to standardise and secure external command execution across the project.
New Features:
ProgramCatalogueinlading.utils.commandsregistering cargo and git, along with exportedCARGO,GIT, andLADING_CATALOGUEsymbols for reuse.lading.utilspackage for convenient access.Enhancements:
Build:
cuprumpackage to the project dependencies and update the lockfile accordingly.Tests: