Implement crate packaging loop for publishable crates - #43
Conversation
![terragon-labs[bot]](https://github.githubassets.com/images/gravatars/gravatar-user-420.png){kind=avatar}
The publish command now runs `cargo package` for every publishable crate in order within the staged workspace. This new implementation prepares crates by packaging them, stopping on any failure and surfacing errors. Publishing to crates.io is deferred to a future milestone. - Introduced internal functions to resolve staged crate roots and package crates - Updated CLI feature tests to verify packaging invocation order - Added unit tests for packaging workflow including success and failure cases - Updated documentation and roadmap to reflect packaging implementation This change enables the publish workflow to validate crate packaging, improving the release pipeline reliability before full publishing support. Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>
|
Warning Rate limit exceeded@leynos has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 6 minutes and 9 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 selected for processing (1)
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 CodeRabbitRelease Notes
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughState the publish workflow now stages the workspace and runs cargo package for every publishable crate in dependency order, halting on the first packaging failure and deferring actual registry publish to a later phase. Changes
Sequence DiagramsequenceDiagram
participant User
participant PublishCmd as Publish command
participant StageWS as Staged workspace
participant Packager as _package_publishable_crates
participant Cargo as cargo (runner)
User->>PublishCmd: invoke publish
PublishCmd->>StageWS: create staged workspace
PublishCmd->>PublishCmd: apply strip-patch strategy
PublishCmd->>Packager: trigger packaging phase
loop for each crate in plan order
Packager->>Packager: resolve staged crate root
Packager->>Cargo: run `cargo package` in staged root
alt success (exit 0)
Cargo-->>Packager: success
else failure (non-zero)
Cargo-->>Packager: error + output
Packager-->>PublishCmd: raise PublishPreflightError (include crate context and output)
PublishCmd-->>User: fail, stop workflow
Note over PublishCmd: staged artifacts retained
end
end
Packager-->>PublishCmd: all packages completed
PublishCmd-->>User: success (publish to registry deferred)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Comment |
Reviewer's GuideImplements a crate-packaging loop in the publish workflow that resolves staged crate roots and runs Sequence diagram for updated publish.run workflow with crate packagingsequenceDiagram
actor Developer
participant CLI as LadingCLI
participant Publish as publish.run
participant Prep as _prepare_workspace_and_plan
participant Package as _package_publishable_crates
participant Runner as _CommandRunner
participant Cargo as cargo
Developer->>CLI: lading publish
CLI->>Publish: run(configuration, workspace_root)
Publish->>Prep: compute PublishPreparation and PublishPlan
Prep-->>Publish: plan, preparation
Publish->>Package: _package_publishable_crates(plan, preparation, runner)
loop for each crate in plan.publishable (in order)
Package->>Package: _resolve_staged_crate_root(crate, plan, staging_root)
Package->>Runner: runner(("cargo", "package"), cwd=crate_root, env=None)
Runner->>Cargo: execute cargo package
Cargo-->>Runner: exit_code, stdout, stderr
Runner-->>Package: exit_code, stdout, stderr
alt exit_code != 0
Package-->>Publish: raise PublishPreflightError(crate, detail)
Publish-->>CLI: propagate error
CLI-->>Developer: show failure with crate context
note right of Package: break packaging loop
else exit_code == 0
Package-->>Package: proceed to next crate
end
end
Note over Publish,CLI: On success, packaging completes for all crates
Publish-->>CLI: formatted plan output
CLI-->>Developer: display publish plan and packaging results
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}
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `tests/unit/publish/test_packaging.py:81-89` </location>
<code_context>
+
+ calls: list[str] = []
+
+ def failing_runner(
+ command: typ.Sequence[str],
+ *,
+ cwd: Path | None = None,
+ env: typ.Mapping[str, str] | None = None,
+ ) -> tuple[int, str, str]:
+ del env, cwd # parameters unused in the stub
+ calls.append(" ".join(command))
+ return (1, "", "packaging failed")
+
+ with pytest.raises(publish.PublishPreflightError) as excinfo:
</code_context>
<issue_to_address>
**suggestion (testing):** Add a unit test that exercises the stdout-vs-stderr selection in the error detail
This test only covers the branch where `stderr` is set and `stdout` is empty, but `_package_publishable_crates` builds the `PublishPreflightError` detail with `(stderr or stdout).strip()`. Please add a companion test where `stderr` is empty and `stdout` contains the failure text to exercise the fallback and guard against regressions in the error-reporting logic.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Added a unit test to verify that when packaging fails, failure details fallback to stdout if stderr is empty. This ensures error messages are properly reported from stdout during publication preflight checks. Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>
![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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (9)
docs/lading-design.md(1 hunks)docs/roadmap.md(1 hunks)docs/usage-guide.md(1 hunks)lading/commands/publish.py(2 hunks)tests/bdd/features/cli.feature(1 hunks)tests/bdd/steps/test_publish_steps.py(3 hunks)tests/unit/publish/conftest.py(5 hunks)tests/unit/publish/test_command_logging.py(4 hunks)tests/unit/publish/test_packaging.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
**/*.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/publish/test_packaging.pylading/commands/publish.pytests/unit/publish/test_command_logging.pytests/bdd/steps/test_publish_steps.pytests/unit/publish/conftest.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/publish/test_packaging.pylading/commands/publish.pytests/unit/publish/test_command_logging.pytests/bdd/steps/test_publish_steps.pytests/unit/publish/conftest.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/publish/test_packaging.pytests/unit/publish/test_command_logging.pytests/bdd/steps/test_publish_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/publish/test_packaging.pytests/unit/publish/test_command_logging.pytests/bdd/steps/test_publish_steps.pytests/unit/publish/conftest.py
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/roadmap.mddocs/usage-guide.mddocs/lading-design.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/roadmap.mddocs/usage-guide.mddocs/lading-design.md
🧬 Code graph analysis (5)
tests/unit/publish/test_packaging.py (1)
lading/commands/publish.py (2)
PublishPreparation(101-105)PublishPreflightError(108-109)
lading/commands/publish.py (4)
lading/workspace/models.py (1)
WorkspaceCrate(59-69)lading/commands/publish_plan.py (1)
PublishPlan(22-34)lading/commands/publish_manifest.py (1)
PublishPreparationError(58-74)lading/commands/publish_execution.py (1)
_CommandRunner(52-62)
tests/unit/publish/test_command_logging.py (1)
tests/unit/publish/conftest.py (1)
use_real_invoke(157-159)
tests/bdd/steps/test_publish_steps.py (1)
crate_tools/unittests/publish_check/conftest.py (1)
env(289-292)
tests/unit/publish/conftest.py (3)
lading/cli.py (1)
publish(284-299)lading/workspace/models.py (3)
WorkspaceCrate(59-69)WorkspaceDependency(50-56)WorkspaceGraph(72-171)lading/commands/publish_execution.py (1)
_invoke(81-94)
🪛 LanguageTool
docs/usage-guide.md
[uncategorized] ~231-~231: Use a comma before ‘so’ if it connects two independent clauses (unless they are closely connected and short).
Context: ...it code and surfaces any captured output so failures remain actionable. Publishing ...
(COMMA_COMPOUND_SENTENCE_2)
docs/lading-design.md
[uncategorized] ~522-~522: Possible missing comma found.
Context: ...follow in a subsequent phase; for > now the workflow stops after successful pac...
(AI_HYDRA_LEO_MISSING_COMMA)
🔍 Remote MCP
Let me try alternative searches to understand the publish workflow and integration points:Based on the Cargo documentation, I now have context about cargo package. Let me search for more information about the testing and error handling patterns specific to this project:Now I have comprehensive context about cargo package and BDD testing. Let me search for information about monkeypatch and fixture patterns in pytest to understand the test infrastructure changes:Perfect! I now have sufficient context to provide a comprehensive review summary. Let me compile the findings.
Summary of Relevant Context Found
Based on my research, here are the key findings that provide valuable context for reviewing this PR:
1. Cargo Package Command Understanding
The recommended practice is to run cargo publish --dry-run (or cargo package which is equivalent) before publishing to ensure there aren't any warnings or errors. This performs verification checks, compression into a .crate file, and extraction/verification by compiling the .crate file from a temporary directory. This validates that the PR's approach of running cargo package as a preflight validation step aligns with established Cargo best practices.
2. Topological Publish Ordering Context
Publishing packages in a Cargo workspace in topological order ensures that dependencies are published before the packages that depend on them, avoiding publishing failures due to missing dependency versions. The PR's implementation of executing packaging in publish order is consistent with this pattern.
3. Pytest Fixture and Monkeypatch Architecture
The test infrastructure changes are aligned with established pytest patterns:
- The monkeypatch fixture helps safely set/delete attributes or modify code for testing, with all modifications undone after the test function finishes
- Fixtures using
autouse=Trueautomatically apply to all tests - The
use_real_invokefixture pattern allows selective tests to restore real behavior while others use mocked behavior, following standard fixture composition patterns
4. BDD Testing Best Practices
Pytest-BDD is a Python-based testing framework that integrates Behavior-Driven Development into the Pytest ecosystem, allowing testers to write test scenarios in Gherkin syntax (Given-When-Then) and map them to Python functions. The PR's additions to BDD feature files and step definitions align with this framework's design.
5. Test Fixture Dependency Injection
Pytest fixtures written for unit tests can be reused for setup and actions mentioned in feature steps with dependency injection, allowing true BDD specification without maintaining context objects. The PR's fixture infrastructure changes support this pattern.
Key Review Considerations:
- The packaging loop implementation correctly invokes
cargo packageas a preflight validation before actual publishing (which is deferred), consistent with Cargo best practices - The error handling with
PublishPreflightErrorprovides immediate feedback on packaging failures - The workspace containment validation (
_resolve_staged_crate_root) prevents packaging crates outside the workspace boundaries - The test infrastructure changes (ORIGINAL_INVOKE, use_real_invoke fixture) follow established pytest patterns for managing mocked vs. real behavior
- The BDD test additions properly validate execution order through preflight invocation recording
[::web_search::], [Ref documentation search]
⏰ 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 (21)
lading/commands/publish.py (2)
307-354: LGTM! Solid defensive programming and error handling.The workspace containment validation in
_resolve_staged_crate_rootprevents packaging crates outside the workspace boundary, and the detailed error messages in_package_publishable_cratesinclude both the crate context and captured output. The(stderr or stdout).strip()fallback ensures failure details are always surfaced.
416-420: Correct integration point for the packaging preflight.Positioning the packaging step after
_apply_strip_patch_strategyand before plan formatting ensures the staged workspace is normalised before validation, and packaging failures abort before reporting success.docs/lading-design.md (1)
519-522: Documentation accurately reflects the new packaging phase.The updated scope note clearly explains that packaging now happens in the staged workspace before any live publish step, with the workflow stopping after successful packaging. The sentence structure is fine; the static analysis hint is a false positive.
docs/roadmap.md (1)
198-198: LGTM! Roadmap reflects the completed milestone.tests/bdd/features/cli.feature (1)
115-120: BDD scenario provides end-to-end validation of packaging order.The new scenario complements the existing publish-order test by verifying that
cargo packageinvocations follow the topological sort, ensuring the packaging preflight respects dependency relationships.docs/usage-guide.md (1)
228-233: Clear documentation of the packaging preflight behavior.The updated usage guide accurately describes the packaging phase, including the stop-on-first-failure behavior and output surfacing. The note that publishing to crates.io is a future milestone sets correct expectations. The static analysis hint about the comma is a false positive.
tests/bdd/steps/test_publish_steps.py (3)
583-596: Verify that deriving crate names from PWD is reliable.The test step extracts crate names by reading
env.get("PWD", "")and usingPath(cwd).name(line 594). This assumes cmd-mox populates PWD in the environment when the runner is invoked with acwdparameter. Whilst this presumably works today, it couples the test to cmd-mox implementation details. If cmd-mox changes how it handlescwd, this test will silently fail by observing empty crate names.Run the following script to confirm the cmd-mox framework sets PWD when cwd is provided:
#!/bin/bash # Description: Verify that recorded invocations include PWD when cwd is set. # Search for cmd-mox invocation recording to understand how cwd maps to env rg -n -A5 -B5 'def record.*cwd' tests/bdd/steps/ rg -n 'PWD.*cwd|cwd.*PWD' tests/If the coupling is confirmed fragile, restructure the test to extract the
cwdparameter directly from the invocation rather than relying on the derived PWD environment variable.
434-442: Helper follows the pattern established by _get_test_invocations.The implementation correctly uses
by_labelto filter cargo::package invocations and raises an assertion error when none are found, maintaining consistency with the existing test helper structure.
223-223: Packaging stub added to the default command set.Adding the cargo package stub ensures the packaging preflight can be exercised in BDD tests without spawning real cargo processes.
tests/unit/publish/test_packaging.py (4)
30-63: Comprehensive test of packaging order and invocation details.The test correctly verifies that
cargo packageruns once per publishable crate in dependency order, with each invocation using the crate's staged root as the working directory.
66-100: Failure handling correctly tested with crate context.The test confirms that packaging aborts on the first failure and includes both the crate name and the captured stderr in the
PublishPreflightErrormessage.
103-134: Stdout fallback tested as requested in past review.This test addresses the previous review comment by exercising the
(stderr or stdout).strip()fallback path, ensuring failure details are surfaced even when stderr is empty.
21-27: Helper correctly mirrors the staged workspace structure.The
_prepare_staging_roothelper creates the staged directory tree expected by the packaging workflow, allowing tests to exercise path resolution without full workspace preparation.tests/unit/publish/conftest.py (3)
139-159: Test infrastructure correctly supports mocked and real invocations.Capturing
ORIGINAL_INVOKEbefore the auto-use fixture stubs it, then providing ause_real_invokefixture to restore the original behavior, follows established pytest patterns for managing mocked vs. real implementations. This allows packaging tests to exercise real subprocess behavior whilst other tests remain fast.Based on learnings, pytest fixtures with
autouse=Trueapply automatically, and fixture composition enables selective restoration of original behavior.
78-85: Test helpers now create filesystem structures for packaging tests.The updated
make_crateandmake_workspacehelpers create actual directories and write minimalCargo.tomlmanifests, enabling packaging tests to exercise path resolution and file existence checks without requiring full workspace fixtures.Also applies to: 110-111
149-153: Stubbing _invoke in the auto-use fixture prevents accidental real subprocess calls.Extending
disable_preflightto stubpublish._invokeensures that tests relying on the default fixture behavior do not spawn real cargo processes, improving test isolation and speed.tests/unit/publish/test_command_logging.py (5)
29-31: LGTM: Correct fixture injection for real invocation behaviour.The
use_real_invokefixture properly restores the original_invokeimplementation for this test to exercise actual subprocess execution and logging. The type annotationNonecorrectly reflects that the fixture performs a side-effect via monkeypatch without returning a value.
43-45: LGTM: Consistent fixture usage for logging verification.The fixture injection matches the pattern in
test_invoke_logs_command_with_cwd, ensuring this test also exercises the real_invokeimplementation to verify logging behaviour whencwdis omitted.
58-60: LGTM: Real invocation required for output streaming verification.The fixture is necessary here to test that
_invokecorrectly streams subprocess stdout/stderr to the parent process. Mocked behaviour cannot verify this integration.
80-85: LGTM: Layered fixture usage correctly tests integration boundary.The combination of
use_real_invoke(restoring realpublish._invoke) followed by monkeypatching_invoke_via_subprocesscorrectly isolates the integration layer. This allows the test to verify that the real_invokeimplementation correctly calls the subprocess runner whilst controlling the subprocess behaviour for test stability.
1-127: LGTM: Complete and consistent fixture adoption across all invocation tests.All four tests in this file now correctly request the
use_real_invokefixture, ensuring they exercise actual subprocess execution rather than mocked behaviour. This complete adoption prevents any tests from inadvertently using the wrong implementation, which aligns with the PR's goal of establishing proper test infrastructure for packaging invocations.The fixture usage is uniform and appropriate: each test that directly calls
publish._invoke()to verify logging, output streaming, or cmd-mox integration now has the real implementation available.
Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
tests/unit/publish/test_packaging.py (1)
51-51: Harmonise parameter deletion across all test runners.The
del envstatement here has no comment, whilst line 87 includes# parameters unused in the stuband line 122 has no comment. Apply a consistent approach: either remove all explanatory comments (thedelstatement is self-documenting) or rename unused parameters to_to signal intent without requiring deletion.Apply this diff to remove the statement entirely by renaming the parameter:
def runner( command: typ.Sequence[str], *, cwd: Path | None = None, - env: typ.Mapping[str, str] | None = None, + _env: typ.Mapping[str, str] | None = None, ) -> tuple[int, str, str]: - del env calls.append((tuple(command), cwd)) return 0, "", ""Also applies to lines 87, 122.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
tests/unit/publish/test_packaging.py(1 hunks)
🧰 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/publish/test_packaging.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/publish/test_packaging.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/publish/test_packaging.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/publish/test_packaging.py
🧬 Code graph analysis (1)
tests/unit/publish/test_packaging.py (2)
tests/unit/publish/conftest.py (2)
make_config(56-67)make_dependency_chain(117-124)lading/commands/publish.py (3)
PublishPreparation(101-105)_package_publishable_crates(330-353)PublishPreflightError(108-109)
🔍 Remote MCP Deepwiki
Summary of additional relevant facts for reviewing this PR
-
New packaging step fits into existing publish pipeline immediately after staging/strip-patches: publish.run performs preflight → plan_publication → prepare_workspace → (now) package publishable crates in staged workspace (packaging is a preflight-only step; live publish still deferred).
-
Command execution is abstracted by a _CommandRunner protocol (callable returning (exitcode, stdout, stderr)); production path uses plumbum, tests use cmd-mox IPC when LADING_USE_CMD_MOX_STUB is set. The packaging helper uses the same runner abstraction (so tests can inject stubs/recorders).
-
Preflight behavior is fail-fast: git status, cargo check, cargo test run in temporary target dir; non-zero exits raise PublishPreflightError which includes subcommand, exit code, and captured output — packaging failures are expected to be surfaced as PublishPreflightError in the same style.
-
Workspace staging semantics: staging copies workspace to a build directory (preserve_symlinks default true), validates staging directory not nested inside workspace, and propagates workspace README to crates with readme.workspace = true — packaging runs in each crate's staged root (resolved relative to staging_root). _resolve_staged_crate_root must ensure crate root is inside staging workspace.
-
Publication planning: publish.order vs topological ordering; package loop must follow PublishPlan.publishable order. plan_publication returns PublishPlan.publishable (ordered tuple) that tests and BDD steps assert against.
-
Error types and messages to expect/verify in tests: PublishPreflightError (preflight/package failures), PublishPlanError (ordering/config errors), PublishPreparationError (staging issues). Packaging helper should include crate name and captured stdout/stderr in error text to match existing patterns.
-
Tests & testing hooks: repo uses unit and BDD tests. Unit tests commonly stub _run_preflight_checks via autouse fixture; tests inject ORIGINAL_INVOKE/use_real_invoke to control command invocation. BDD tests run CLI subprocesses and rely on cmd-mox to record/verify cargo invocations and order — the added BDD scenario and step-recording of cargo package invocations align with that approach. Ensure new packaging invocations are normalized similarly to other cargo subcommands for cmd-mox.
-
Output & formatting expectations: publish.run prints plan + staging summary; packaging failures are expected to interrupt flow early and surface human-readable diagnostic consistent with existing _build_cargo_error_message formatting. Tests assert on CLI stdout/stderr and recorded invocations.
⏰ 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 (3)
tests/unit/publish/test_packaging.py (3)
1-19: Module structure and imports look good.The module follows Python best practices: future annotations, TYPE_CHECKING guard for Path, standard import aliases, and a clear docstring.
21-27: Helper function correctly mirrors staging structure.The
_prepare_staging_roothelper creates the expected directory layout for staged crates, matching the logic in_resolve_staged_crate_root. Type annotations and pathlib usage are correct.
66-100: Fail-fast behaviour correctly verified.The test confirms that
_package_publishable_cratesaborts on the first failure and includes the crate name and stderr detail in thePublishPreflightError. The stdout fallback path is exercised bytest_package_publishable_crates_reports_stdout_on_failure.
Add a new unit test to verify that when both stderr and stdout have error details during package publishing, the error reporting correctly prefers stderr output over stdout. This ensures clearer error diagnostics for publish failures. Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
|
✅ Resolved |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
tests/unit/publish/test_packaging.py(1 hunks)
🧰 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/publish/test_packaging.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/publish/test_packaging.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/publish/test_packaging.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/publish/test_packaging.py
🧬 Code graph analysis (1)
tests/unit/publish/test_packaging.py (2)
lading/commands/publish_plan.py (2)
PublishPlan(22-34)plan_publication(146-195)lading/commands/publish.py (3)
PublishPreparation(101-105)_package_publishable_crates(330-353)PublishPreflightError(108-109)
🔍 Remote MCP Deepwiki
Relevant additional facts for reviewing PR #43 (concise):
-
Where packaging is invoked
- Packaging is inserted after staging/strip-patches in publish.run and uses the same _CommandRunner protocol as preflight (callable returning (exitcode, stdout, stderr)).
-
Command execution & test stubbing
- Production invokes commands via plumbum; tests/BDD use cmd-mox when LADING_USE_CMD_MOX_STUB is truthy; cargo subcommands are normalized to e.g. "cargo::check" for cmd-mox. Tests inject a runner or use cmd-mox to record/return responses. Ensure _package_publishable_crates uses the injected runner and will be observable by existing test stubs/recorders.
-
Error types & messages expectations
- Failures in preflight/package phases raise PublishPreflightError with subcommand, exit code, and captured output; PublishPreparationError / PublishPlanError used elsewhere. Packaging errors should follow same message pattern (crate context + stdout/stderr).
-
Staging semantics for packaging
- Packaging must run in each crate's staged root (staging_root / crate.relative_path); staging creation validates build_directory not inside workspace and preserves symlink behavior by PublishOptions.preserve_symlinks. _resolve_staged_crate_root must ensure staged path is inside staging workspace.
-
Publish plan ordering
- Packaging must iterate PublishPlan.publishable in the plan order (configured order or topological); tests assert publishable_names ordering.
-
Tests added / test integration points
- Unit tests for packaging (ordering, failure propagation, stdout in error) are added; tests use fixtures in tests/unit/publish/conftest.py (ORIGINAL_INVOKE, use_real_invoke) and autouse preflight disabling. BDD adds scenario and step helpers that record cargo package invocations via preflight recorder — ensure naming/normalization of cargo package invocations matches cmd-mox conventions.
-
Logging / output formatting
- Publish.run formats plan + staging summary; packaging failures should surface via existing CLI error handling (printed by main with consistent exit codes).
Tools used:
- Deepwiki_read_wiki_structure
- Deepwiki_read_wiki_contents
⏰ 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 (6)
tests/unit/publish/test_packaging.py (6)
21-27: Keep helper mirroring staged workspace layout.Keep
_prepare_staging_rootas the focused helper that mirrors the staging layout used by_package_publishable_crates; it sets up the minimal tree required for these tests without leaking extra concerns.
30-63: Retain precise assertion of command order and cwd.Retain this test as-is; it validates both the plan ordering and that each
cargo packageinvocation runs in the correct staged crate root, which is exactly the observable contract of_package_publishable_crates.
66-101: Keep explicit check that packaging aborts on first failure.Keep this test; it tightly checks that the workflow aborts after the first failing crate and that the
PublishPreflightErrormessage includes both the crate name and stderr detail, which is the expected user-facing behaviour.
103-135: Keep stdout‑fallback failure coverage.Keep this test; it exercises the
(stderr or stdout).strip()fallback path by ensuring stdout details appear in the error message when stderr is empty, which guards against regressions in error reporting.
137-168: Keep stderr‑preference coverage when both streams are populated.Keep this test; it completes the matrix by asserting that stderr is preferred over stdout when both are populated, and that stdout content does not leak into the final message.
1-168: Run publish‑focused tests and Python tooling.Run the publish‑focused tests and Python tooling for this module to confirm everything passes under the project’s pipelines.
#!/bin/bash set -euo pipefail # From repo root pytest -k publish # Python tooling (follow project make targets) make lint make check-fmt make typecheck
…type checking Replaced typing.Sequence and typing.Mapping with collections.abc.Sequence and collections.abc.Mapping in tests/unit/publish/test_packaging.py to enhance type checking clarity and adhere to typing best practices in test code. Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>
1 participant
Summary
Adds a dedicated crate-packaging loop that runs cargo package for every publishable crate in publish order inside a staged workspace. This lays the groundwork for the publish step by validating packaging behavior without performing an actual publish yet.
Changes
Core functionality
Tests
-BDD / CLI tests:
Documentation
Test plan
Rationale
This change implements the crate packaging loop as a core preflight step, ensuring every publishable crate can be validated in isolation within a staged workspace before any publish action. It provides clear failure messaging tied to the crate being packaged and prepares the system for a subsequent publish command without changing the current default behavior (dry-run packaging).
🌿 Generated by Terry
ℹ️ Tag @terragon-labs to ask questions and address PR feedback
📎 Task: https://www.terragonlabs.com/task/05d1f854-9b0d-48e4-90bf-920c9b561181
Summary by Sourcery
Add a crate-packaging preflight step to the publish command that runs
cargo packagefor each publishable crate in a staged workspace and validates packaging before any actual publish.New Features:
cargo packageper publishable crate in publish order within the staged workspace, aborting on failures with clear crate-specific errors.Enhancements:
_invokehelper during tests.Documentation:
Tests:
cargo packageis invoked for each publishable crate in the correct order and to record packaging invocations for verification.Chores:
use_real_invokefixture and ensure command logging and cmd-mox passthrough still exercise the actual subprocess invocation path.