Phase 4: expand tests, tooling, docs, and IPC testing - #45
Conversation
![terragon-labs[bot]](https://github.githubassets.com/images/gravatars/gravatar-user-420.png){kind=avatar}
…ands - Added comprehensive unit tests for publish diagnostics, execution helpers, manifest strategies, command shared helpers, config, toml utils, workspace metadata, and workspace models validation. - Introduced pytest-cov to dev dependencies for coverage reporting. - Updated documentation to reflect enhanced testing strategy. - Improved testing coverage of error paths and IPC mechanisms in publish command implementation. - Aim to ensure >90% line coverage for new modules and maintain robustness. Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.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. WalkthroughAdd pytest-cov and many unit/BDD tests; annotate TYPE_CHECKING blocks with Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant User
participant CLI as lading CLI
participant Publish as publish module
participant Preflight as Preflight / CmdMox
participant Cargo as cargo subprocess
Note over User,CLI: User invokes publish command
User->>CLI: run publish
CLI->>Publish: build PublishPlan & PublishPreparation
CLI->>Publish: construct _PublishExecutionOptions(live, allow_dirty)
Publish->>Preflight: execute preflight (normalise commands, pass allow_dirty)
Preflight-->>Publish: preflight results (may mutate env via cmd-mox)
Publish->>Cargo: cargo package (args include --allow-dirty if set)
Cargo-->>Publish: package result
Publish->>Cargo: cargo publish (include --allow-dirty; add --dry-run when not live)
Cargo-->>Publish: publish result
Publish-->>CLI: report outcomes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit 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:
🧬 Code graph analysis (1)tests/bdd/steps/test_publish_infrastructure.py (1)
🔍 Remote MCP DeepwikiSummary of additional facts relevant to reviewing this PR
Tools/sources used
🔇 Additional comments (6)
Comment |
Reviewer's GuideAdds Phase 4-focused unit tests for configuration, publish execution/diagnostics/manifest strategies, workspace metadata and models; wires in pytest-cov for coverage; and updates documentation and typing pragmas to reflect the stabilized testing and IPC/cmd-mox strategy. 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/unit/publish/test_publish_execution_helpers.py Comment on lines +76 to +157 def test_handle_cmd_mox_passthrough_reports_response(
monkeypatch: pytest.MonkeyPatch, tmp_path: Path
) -> None:
"""Passthrough directives resolved to responses should be reported back."""
socket_path = str(tmp_path / "cmox" / "shim" / "socket")
monkeypatch.setenv("CMOX_IPC_SOCKET", socket_path)
class _Env:
CMOX_IPC_SOCKET_ENV = "CMOX_IPC_SOCKET"
CMOX_REAL_COMMAND_ENV_PREFIX = "CMOX_REAL_"
class _IPC:
class Response:
def __init__(
self, stdout: str = "", stderr: str = "", exit_code: int = 0
) -> None:
self.stdout = stdout
self.stderr = stderr
self.exit_code = exit_code
class PassthroughResult:
def __init__(
self, invocation_id: str, stdout: str, stderr: str, exit_code: int
) -> None:
self.invocation_id = invocation_id
self.stdout = stdout
self.stderr = stderr
self.exit_code = exit_code
def report_passthrough_result(self, result: object, timeout: float) -> Response:
return self.Response(
stdout=getattr(result, "stdout", ""),
stderr=getattr(result, "stderr", ""),
exit_code=getattr(result, "exit_code", 0),
)
class _CommandRunner:
def prepare_environment(
self,
lookup_path: str,
extra_env: dict[str, str],
invocation_env: dict[str, str],
) -> dict[str, str]:
shim_dir = Path(socket_path).parent
env = {"PATH": f"{lookup_path}{os.pathsep}{shim_dir}{os.pathsep}/usr/bin"}
env.update(extra_env)
env.update(invocation_env)
return env
def resolve_command_with_override(
self, command: str, path: str, override: str | None
) -> _IPC.Response:
return _IPC.Response(stdout="pass", stderr="through", exit_code=0)
directive = SimpleNamespace(
invocation_id="123",
lookup_path=str(tmp_path / "cmox" / "bin"),
extra_env={"EXTRA": "1"},
)
invocation = SimpleNamespace(
env={"PATH": str(tmp_path / "cmox" / "bin")},
command="cargo",
args=("test",),
stdin="",
)
modules = publish_execution.CmdMoxModules(
ipc=_IPC(),
env=_Env,
command_runner=_CommandRunner(),
)
response = SimpleNamespace(passthrough=directive)
returned, streamed = publish_execution._handle_cmd_mox_passthrough(
response,
invocation,
timeout=1.0,
modules=modules,
)
assert streamed is False
assert isinstance(returned, _IPC.Response)
assert returned.stdout == "pass"❌ New issue: Large Method |
|
@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/unit/publish/test_publish_manifest_strategies.py Comment on lines +33 to +49 def test_apply_strip_patch_strategy_removes_all_entries(tmp_path: Path) -> None:
"""The 'all' strategy should drop the entire patch table."""
manifest_path = tmp_path / "Cargo.toml"
_write_manifest(
manifest_path,
"""
[patch.crates-io]
alpha = { path = "../alpha" }
serde = { git = "https://example.com/serde" }
""",
)
plan = _make_plan(tmp_path, ("alpha",))
publish_manifest._apply_strip_patch_strategy(tmp_path, plan, "all")
document = tomlkit.parse(manifest_path.read_text(encoding="utf-8"))
assert "patch" not in document❌ New issue: Code Duplication |
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
- Introduced pytest fixtures in test_publish_execution_helpers.py to stub cmd-mox environment, IPC, and command runner modules improving test isolation and reuse. - Updated tests in test_publish_execution_helpers.py to utilize these fixtures instead of local classes. - Added a reusable helper in test_publish_manifest_strategies.py to reduce duplication when testing strip patch strategies. - Simplified existing tests for _apply_strip_patch_strategy by using the helper, improving readability and maintainability. 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.
|
@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/unit/publish/test_publish_execution_helpers.py Comment on lines +148 to +186 def test_handle_cmd_mox_passthrough_reports_response(
monkeypatch: pytest.MonkeyPatch,
tmp_path: Path,
mock_cmd_mox_env_module: type,
mock_cmd_mox_ipc_module: type,
mock_cmd_mox_command_runner: type,
) -> None:
"""Passthrough directives resolved to responses should be reported back."""
socket_path = str(tmp_path / "cmox" / "shim" / "socket")
monkeypatch.setenv("CMOX_IPC_SOCKET", socket_path)
directive = SimpleNamespace(
invocation_id="123",
lookup_path=str(tmp_path / "cmox" / "bin"),
extra_env={"EXTRA": "1"},
)
invocation = SimpleNamespace(
env={"PATH": str(tmp_path / "cmox" / "bin")},
command="cargo",
args=("test",),
stdin="",
)
modules = publish_execution.CmdMoxModules(
ipc=mock_cmd_mox_ipc_module(),
env=mock_cmd_mox_env_module,
command_runner=mock_cmd_mox_command_runner(),
)
response = SimpleNamespace(passthrough=directive)
returned, streamed = publish_execution._handle_cmd_mox_passthrough(
response,
invocation,
timeout=1.0,
modules=modules,
)
assert streamed is False
assert isinstance(returned, mock_cmd_mox_ipc_module.Response)
assert returned.stdout == "pass"❌ New issue: Excess Number of Function Arguments |
This comment was marked as resolved.
This comment was marked as resolved.
Refactored cmd-mox related pytest fixtures in test_publish_execution_helpers.py by combining environment, IPC, and command runner stubs into a single mock_cmd_mox_modules fixture. This improves test code organization and reuse, and updates relevant tests to use the unified fixture. Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- The directory structure example in docs/lading-design.md was unintentionally flattened into a single wrapped line inside the code block; restoring the previous multiline tree formatting will make it readable again.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The directory structure example in docs/lading-design.md was unintentionally flattened into a single wrapped line inside the code block; restoring the previous multiline tree formatting will make it readable again.
## Individual Comments
### Comment 1
<location> `tests/unit/test_workspace_metadata.py:426-431` </location>
<code_context>
+ metadata_module._CmdMoxCommand().run()
+
+
+def test_resolve_cmd_mox_timeout_validates_values() -> None:
+ """Timeout parsing should reject non-positive values."""
+ assert metadata_module._resolve_cmd_mox_timeout(None) > 0
+ assert metadata_module._resolve_cmd_mox_timeout("2.5") == 2.5
+ with pytest.raises(metadata_module.CargoMetadataError):
+ metadata_module._resolve_cmd_mox_timeout("0")
+
</code_context>
<issue_to_address>
**suggestion (testing):** Add tests for non-numeric and negative timeout values to fully cover `_resolve_cmd_mox_timeout` error paths.
The existing test already covers `None`, a valid float string, and the zero-value error path. To fully specify the behavior, please also add cases for a non-numeric value (e.g. `"abc"`) and a negative numeric value (e.g. `"-1"`) and assert they raise `CargoMetadataError`, so callers can rely on consistent handling of arbitrary environment-derived strings.
```suggestion
def test_resolve_cmd_mox_timeout_validates_values() -> None:
"""Timeout parsing should reject non-positive and non-numeric values."""
# Defaults to a positive timeout when unset
assert metadata_module._resolve_cmd_mox_timeout(None) > 0
# Accepts valid numeric string
assert metadata_module._resolve_cmd_mox_timeout("2.5") == 2.5
# Rejects zero, negative, and non-numeric values
for value in ("0", "-1", "abc"):
with pytest.raises(metadata_module.CargoMetadataError):
metadata_module._resolve_cmd_mox_timeout(value)
```
</issue_to_address>
### Comment 2
<location> `tests/unit/test_toml_utils.py:25-31` </location>
<code_context>
+ assert list(document_obj) == []
+
+
+def test_ensure_table_rejects_non_table_values() -> None:
+ """A non-table entry under the key should raise an assertion."""
+ doc = document()
+ doc["publish"] = "invalid"
+
+ with pytest.raises(AssertionError, match="publish must be a table"):
+ toml_utils.ensure_table(doc, "publish")
+
+
</code_context>
<issue_to_address>
**suggestion (testing):** Add a positive-path test for `ensure_table` creating or returning a valid table.
Currently we only cover the failure case when `ensure_table` sees a non-table value. Please also add a happy-path test that starts from an empty document, calls `ensure_table(doc, "publish")`, and asserts that it returns a TOML table and updates the document accordingly. That will exercise both creation and retrieval behaviour and protect future changes to this helper.
```suggestion
def test_ensure_table_rejects_non_table_values() -> None:
"""A non-table entry under the key should raise an assertion."""
doc = document()
doc["publish"] = "invalid"
with pytest.raises(AssertionError, match="publish must be a table"):
toml_utils.ensure_table(doc, "publish")
def test_ensure_table_creates_and_returns_table() -> None:
"""Missing keys should be initialised to a TOML table and returned."""
doc = document()
table_obj = toml_utils.ensure_table(doc, "publish")
# The key should now exist in the document and reference the same table object
assert "publish" in doc
assert doc["publish"] is table_obj
# The created object should be a TOML table, not a plain dict
assert isinstance(table_obj, type(table()))
```
</issue_to_address>
### Comment 3
<location> `docs/lading-design.md:575-576` </location>
<code_context>
+
+### Phase 4 testing updates
+
+- Introduced `pytest-cov` as a development dependency so coverage can be
+ reported via `uv run pytest --cov` without additional tooling. Phase 4 sets a
+ floor of >90% line coverage for new modules; focused unit tests now exercise
+ configuration validation edges, publish manifest handling, cmd-mox IPC
+ fallbacks, and workspace model error paths to keep defensive code paths
+ observable.
+- Cmd-mox remains the default mechanism for mocking external commands. Tests
+ covering publish pre-flight and `cargo metadata` IPC use stubbed cmd-mox
+ modules rather than spawning real processes, keeping suites deterministic
</code_context>
<issue_to_address>
**nitpick (typo):** Align capitalization of `cmd-mox` across the new bullets.
The first bullet uses `cmd-mox IPC fallbacks` (lowercase `cmd`), while the second starts with `Cmd-mox remains...` (capital `C`). Please pick one capitalization that matches the official tool name and use it consistently in both bullets.
```suggestion
- cmd-mox remains the default mechanism for mocking external commands. Tests
covering publish pre-flight and `cargo metadata` IPC use stubbed cmd-mox
```
</issue_to_address>
### Comment 4
<location> `docs/lading-design.md:572` </location>
<code_context>
+- Introduced `pytest-cov` as a development dependency so coverage can be
+ reported via `uv run pytest --cov` without additional tooling. Phase 4 sets a
+ floor of >90% line coverage for new modules; focused unit tests now exercise
+ configuration validation edges, publish manifest handling, cmd-mox IPC
+ fallbacks, and workspace model error paths to keep defensive code paths
+ observable.
</code_context>
<issue_to_address>
**issue (review_instructions):** The acronym “IPC” is introduced without being expanded on first use, which violates the requirement to define uncommon acronyms on first use.
Please expand “IPC” on its first occurrence (for example, “inter-process communication (IPC)”) so the acronym is defined when it is first introduced in the document.
<details>
<summary>Review instructions:</summary>
**Path patterns:** `**/*.md`
**Instructions:**
Define uncommon acronyms on first use.
</details>
</issue_to_address>
### Comment 5
<location> `tests/unit/publish/test_publish_execution_helpers.py:52-61` </location>
<code_context>
@pytest.fixture
def mock_cmd_mox_modules(tmp_path: Path) -> SimpleNamespace:
"""Provide complete cmd-mox module stubs for passthrough handling."""
class _Env:
CMOX_IPC_SOCKET_ENV = "CMOX_IPC_SOCKET"
CMOX_REAL_COMMAND_ENV_PREFIX = "CMOX_REAL_"
class _IPC:
class Response:
def __init__(
self, stdout: str = "", stderr: str = "", exit_code: int = 0
) -> None:
self.stdout = stdout
self.stderr = stderr
self.exit_code = exit_code
class PassthroughResult:
def __init__(
self, invocation_id: str, stdout: str, stderr: str, exit_code: int
) -> None:
self.invocation_id = invocation_id
self.stdout = stdout
self.stderr = stderr
self.exit_code = exit_code
def report_passthrough_result(self, result: object, timeout: float) -> Response:
return self.Response(
stdout=getattr(result, "stdout", ""),
stderr=getattr(result, "stderr", ""),
exit_code=getattr(result, "exit_code", 0),
)
class _CommandRunner:
def prepare_environment(
self,
lookup_path: str,
extra_env: dict[str, str],
invocation_env: dict[str, str],
) -> dict[str, str]:
shim_dir = (tmp_path / "cmox" / "shim").parent
env = {"PATH": f"{lookup_path}{os.pathsep}{shim_dir}{os.pathsep}/usr/bin"}
env.update(extra_env)
env.update(invocation_env)
return env
def resolve_command_with_override(
self, command: str, path: str, override: str | None
) -> _IPC.Response:
return _IPC.Response(stdout="pass", stderr="through", exit_code=0)
return SimpleNamespace(
env_module=_Env,
ipc_module=_IPC,
command_runner=_CommandRunner,
)
</code_context>
<issue_to_address>
**issue (code-quality):** Merge dictionary updates via the union operator [×4] ([`dict-assign-update-to-union`](https://docs.sourcery.ai/Reference/Default-Rules/suggestions/dict-assign-update-to-union/))
</issue_to_address>
### Comment 6
<location> `tests/unit/test_toml_utils.py:22` </location>
<code_context>
def test_load_or_create_document_initialises_empty_document(tmp_path: Path) -> None:
"""New documents should be created when the config file is absent."""
config_path = tmp_path / "lading.toml"
document_obj = toml_utils.load_or_create_document(config_path)
assert list(document_obj) == []
</code_context>
<issue_to_address>
**suggestion (code-quality):** Replaces an empty collection equality with a boolean operation ([`simplify-empty-collection-comparison`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/simplify-empty-collection-comparison/))
```suggestion
assert not list(document_obj)
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pyproject.toml (1)
29-38: Phase 4 coverage workflow requires CI update to align with designThe review correctly identifies
pytest-covfor Phase 4, but implementation is incomplete. The current CI workflow usesslipcoverwithpytest --forkedforcrate_toolscoverage only. Phase 4 design expects coverage to be reported viauv run pytest --covwithout additional tooling, and the >90% target applies to "all new modules"—which should include theladingpackage.Update the CI workflow to:
- Replace the slipcover approach with pytest-cov
- Extend coverage measurement to both
crate_toolsandladingpackages- Verify that overall line coverage meets the >90% threshold before marking Phase 4 complete
pytest-cov is compatible with Python 3.13 and the project's uv setup.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (18)
docs/lading-design.md(4 hunks)docs/roadmap.md(1 hunks)docs/usage-guide.md(1 hunks)lading/commands/_shared.py(1 hunks)lading/commands/publish_manifest.py(1 hunks)lading/config.py(1 hunks)lading/testing/toml_utils.py(1 hunks)lading/utils/process.py(1 hunks)lading/workspace/metadata.py(1 hunks)pyproject.toml(1 hunks)tests/unit/publish/test_publish_diagnostics.py(1 hunks)tests/unit/publish/test_publish_execution_helpers.py(1 hunks)tests/unit/publish/test_publish_manifest_strategies.py(1 hunks)tests/unit/test_command_shared.py(1 hunks)tests/unit/test_config.py(1 hunks)tests/unit/test_toml_utils.py(1 hunks)tests/unit/test_workspace_metadata.py(2 hunks)tests/unit/test_workspace_models_validation.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
**/*.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/process.pylading/config.pylading/workspace/metadata.pytests/unit/test_workspace_models_validation.pytests/unit/test_command_shared.pytests/unit/publish/test_publish_diagnostics.pytests/unit/publish/test_publish_execution_helpers.pytests/unit/test_toml_utils.pytests/unit/publish/test_publish_manifest_strategies.pylading/commands/publish_manifest.pylading/testing/toml_utils.pytests/unit/test_workspace_metadata.pytests/unit/test_config.pylading/commands/_shared.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/process.pylading/config.pylading/workspace/metadata.pytests/unit/test_workspace_models_validation.pytests/unit/test_command_shared.pytests/unit/publish/test_publish_diagnostics.pytests/unit/publish/test_publish_execution_helpers.pytests/unit/test_toml_utils.pytests/unit/publish/test_publish_manifest_strategies.pylading/commands/publish_manifest.pylading/testing/toml_utils.pytests/unit/test_workspace_metadata.pytests/unit/test_config.pylading/commands/_shared.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/lading-design.mddocs/usage-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/roadmap.mddocs/lading-design.mddocs/usage-guide.md
**/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/test_workspace_models_validation.pytests/unit/test_command_shared.pytests/unit/publish/test_publish_diagnostics.pytests/unit/publish/test_publish_execution_helpers.pytests/unit/test_toml_utils.pytests/unit/publish/test_publish_manifest_strategies.pytests/unit/test_workspace_metadata.pytests/unit/test_config.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/test_workspace_models_validation.pytests/unit/test_command_shared.pytests/unit/publish/test_publish_diagnostics.pytests/unit/publish/test_publish_execution_helpers.pytests/unit/test_toml_utils.pytests/unit/publish/test_publish_manifest_strategies.pytests/unit/test_workspace_metadata.pytests/unit/test_config.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
🧬 Code graph analysis (6)
tests/unit/test_workspace_models_validation.py (1)
lading/workspace/models.py (17)
WorkspaceDependency(50-56)_is_ordering_dependency(21-30)WorkspaceModelError(33-34)build_workspace_graph(184-215)_index_workspace_packages(218-230)_build_dependencies(261-281)_validate_dependency_mapping(284-291)_validate_dependency_kind(312-327)_lookup_workspace_target(294-309)_normalise_workspace_root(354-363)_normalise_manifest_path(366-372)_expect_sequence(375-392)_expect_string(395-400)_is_non_empty_sequence(403-409)_coerce_publish_setting(412-425)_extract_readme_workspace_flag(428-436)_manifest_uses_workspace_readme(439-452)
tests/unit/test_command_shared.py (1)
lading/commands/_shared.py (1)
describe_crates(11-15)
tests/unit/publish/test_publish_diagnostics.py (2)
lading/commands/publish_diagnostics.py (3)
_append_compiletest_diagnostics(56-79)_read_tail_lines(29-38)_format_artifact_diagnostics(41-53)tests/unit/test_workspace_metadata.py (1)
_raise(114-115)
tests/unit/publish/test_publish_manifest_strategies.py (2)
lading/commands/publish_plan.py (2)
PublishPlan(22-34)publishable_names(32-34)lading/commands/publish_manifest.py (2)
_apply_strip_patch_strategy(182-205)PublishPreparationError(58-74)
tests/unit/test_workspace_metadata.py (1)
lading/workspace/metadata.py (12)
CargoMetadataError(22-33)_CmdMoxCommand(126-158)run(136-158)_resolve_cmd_mox_timeout(184-194)_build_invocation_environment(161-168)_coerce_text(97-101)invalid_cmd_mox_timeout(26-28)non_positive_cmd_mox_timeout(31-33)_load_cmd_mox_modules(171-181)_build_cmd_mox_command(197-199)_ensure_command(86-94)argv(132-134)
tests/unit/test_config.py (1)
lading/config.py (16)
PreflightConfig(109-167)from_mapping(42-51)from_mapping(62-72)from_mapping(84-97)from_mapping(120-167)from_mapping(179-194)CompiletestExtern(101-105)ConfigurationError(23-24)_validate_mapping_keys(202-227)_string_tuple(294-306)_string_matrix(340-351)_string_mapping(354-364)_optional_mapping(407-416)_non_negative_int(367-379)_boolean(382-389)_strip_patches(392-404)
🪛 LanguageTool
docs/lading-design.md
[uncategorized] ~569-~569: Use a comma before ‘so’ if it connects two independent clauses (unless they are closely connected and short).
Context: ...pytest-cov as a development dependency so coverage can be reported via `uv run ...
(COMMA_COMPOUND_SENTENCE_2)
docs/usage-guide.md
[style] ~240-~240: Would you like to use the Oxford spelling “normalizes”? The spelling ‘normalises’ is also correct.
Context: ...r staging the workspace, publish also normalises the root Cargo.toml according to the ...
(OXFORD_SPELLING_Z_NOT_S)
🔍 Remote MCP
Based on my research of the lading repository, here is a concise summary of the relevant context for reviewing this PR:
PR Context Summary
PR Objective: Finalize Phase 4 testing scaffolding and enhance documentation. The PR adds comprehensive unit tests, pytest-cov for coverage reporting, and documentation updates reflecting testing strategies and Phase 4 stability efforts.
Key Changes Identified
Testing Infrastructure:
-
New test modules: Comprehensive unit tests added for:
- Configuration validation (
tests/unit/test_config.py) - Publish manifest strategies (
tests/unit/publish/test_publish_manifest_strategies.py) - Publish execution helpers (
tests/unit/publish/test_publish_execution_helpers.py) - Preflight checks and diagnostics (
tests/unit/publish/test_publish_diagnostics.py,test_preflight_checks.py) - Workspace model validation (
tests/unit/test_workspace_models_validation.py) - TOML utilities (
tests/unit/test_toml_utils.py) - Workspace metadata (
tests/unit/test_workspace_metadata.py)
- Configuration validation (
-
pytest-cov integration: Added as dev dependency for test coverage reporting (enable via
uv run pytest --cov) -
Testing patterns: Emphasizes:
- Cmd-mox for IPC-based external command stubbing (replacing real
cargo/git) - Unit tests with mocked dependencies
- BDD tests for end-to-end workflows
- Phase 4 goal: >90% line coverage
- Cmd-mox for IPC-based external command stubbing (replacing real
Documentation Updates:
docs/lading-design.md: Phase 4 testing approach and IPC/mox patternsdocs/roadmap.md: Marking test coverage tasks as completedocs/usage-guide.md: Clarified publish workflow, patch stripping options, pre-flight checks, and README propagation
Code Quality:
- Added
pragma: no covercomments to TYPE_CHECKING import blocks across multiple modules to improve coverage reporting accuracy (these blocks don't execute at runtime)
Testing Strategy Employed
The PR establishes a dual-testing approach:
- Unit tests (tests/unit/): Direct function invocation with mocks and temporary workspace fixtures
- BDD tests (tests/bdd/): End-to-end CLI subprocess invocation with cmd-mox command stubbing
Key testing patterns:
- Use of factory fixtures (
make_crate,make_workspace,make_config) for test workspace construction - Environment-based command stubbing (
LADING_USE_CMD_MOX_STUB=1,CMOX_IPC_SOCKET) - Automatic preflight check disabling in unit tests via autouse fixture
- Configuration validation and error path testing
Quality Metrics
- Phase 4 coverage target: >90% line coverage
- Test timeouts: 30 seconds (enforced by pytest-timeout)
- All tests must pass
make allquality gates (format, lint, typecheck, tests)
⏰ 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)
lading/workspace/metadata.py (1)
16-20: Keep TYPE_CHECKING imports excluded from coverageMaintain this
# pragma: no coveron thetyp.TYPE_CHECKINGblock; it correctly marks these imports as type-only and leaves runtime behaviour and exception paths unchanged.lading/utils/process.py (1)
9-15: Retain type-only Logger/Path aliases outside coverage metricsKeep this pattern of real imports under
typ.TYPE_CHECKINGwithLoggerType/PathTypefalling back totyp.Anyat runtime and excluded from coverage; it satisfies typing and RuffTCrules without affecting logging behaviour.lading/commands/_shared.py (1)
7-8: Exclude WorkspaceGraph typing helper from coverageKeep this
# pragma: no coveron theWorkspaceGraphTYPE_CHECKING import; it correctly treats the import as a typing helper only and leavesdescribe_cratesbehaviour untouched.docs/roadmap.md (1)
221-233: Validate coverage before marking “Achieve High Test Coverage” as completeBack this
[x] Achieve High Test Coveragetick with an actual coverage run on the current branch. Generate a report with pytest-cov and confirm that the documented threshold (over 90% line coverage for all new modules) is met before merging this roadmap change.lading/config.py (1)
15-17: Keep configuration TYPE_CHECKING imports out of coverageRetain this
# pragma: no coverannotation on thePathTYPE_CHECKING block; it standardizes treatment of type-only imports across the codebase and does not alter configuration loading or validation semantics.tests/unit/test_workspace_metadata.py (2)
39-392: Exercise cargo-metadata happy paths, error paths, and workspace modelling thoroughlyKeep this suite of tests around
load_cargo_metadata,build_workspace_graph, andload_workspace. The parametrised scenarios, byte/text handling checks, logging assertions, and WorkspaceGraph shape validations collectively give strong coverage of success cases and error surfaces without over-mocking the internals.
393-536: Keep cmd-mox IPC stub tests to guard the new proxy behaviourRetain these cmd-mox–focused tests that validate socket presence, timeout parsing, environment construction, IPC invocation wiring, and convenience error constructors. The lightweight
_StubEnv/_StubIPCstand-ins and direct assertions againstcommand.argv,ipc.last_invocation, andipc.timeoutgive precise coverage of_CmdMoxCommand.runand helpers without depending on real cmd-mox IPC.tests/unit/test_command_shared.py (1)
1-16: Keep this focused pluralisation test fordescribe_cratesLeave this test in place; it cleanly exercises both the singular and plural branches of
describe_cratesusing a minimalSimpleNamespaceworkspace stand-in and aligns with the helper’s documented behaviour.tests/unit/test_workspace_models_validation.py (1)
1-118: Good coverage of workspace model validation paths.The tests exercise ordering dependencies, graph construction, indexing, dependency handling, path normalisation, coercion helpers, and readme flag extraction. This aligns well with the Phase 4 coverage goals.
tests/unit/test_config.py (5)
201-223: LGTM — comprehensive coverage of extended preflight configuration fields.The test validates
aux_build,compiletest_extern,env, andstderr_tail_linesnormalisation, confirming the mapping-to-dataclass transformation works as expected.
225-238: Good validation of unknown-key detection.These tests confirm
_validate_mapping_keysraises descriptive errors for unknown sections and gracefully handlesNonemappings.
241-260: String helper tests exercise acceptance and rejection paths appropriately.The coverage for
_string_tuple,_string_matrix,_string_mapping, and_optional_mappinghits both valid inputs and type-error conditions.
263-283: Numeric and boolean normalisation tests are thorough.The tests cover default fallback, string-to-int coercion, negative-value rejection, and boolean type enforcement. The
strip_patchestests correctly verify rejection ofTrueand unknown string values.
288-305: Nested context restoration test validates LIFO semantics.The test confirms that exiting an inner
use_configurationcontext restores the outer configuration, and exiting all contexts raisesConfigurationNotLoadedError.docs/usage-guide.md (2)
232-238: Documentation accurately describes the staged publish workflow.The clarification that
cargo packageruns in plan order inside the staged copy, followed bycargo publish --dry-runper crate, matches the implementation. The warning-and-continue behaviour for already-published versions is documented correctly.
240-251: Strip-patches strategy documentation is clear and complete.The three strategies (
"all","per-crate",false) are explained with their use cases. The note about staged manifests leaving the original workspace untouched addresses a common concern.lading/testing/toml_utils.py (1)
33-33: LGTM — pragma annotation added for coverage accuracy.The
TYPE_CHECKINGblock correctly receives thepragma: no covercomment, consistent with other modules in this PR.lading/commands/publish_manifest.py (1)
42-42: LGTM — pragma annotation added for coverage accuracy.The
TYPE_CHECKINGblock correctly receives thepragma: no covercomment, aligning with the Phase 4 coverage-tooling improvements.tests/unit/test_toml_utils.py (1)
1-74: Keep TOML utility tests as writtenLeave the structure and expectations of these tests in place; they exercise the key behaviours of
toml_utilswell (document initialisation, assertion messages, manifest loading, and workspace/crate resolution) and align with the stated Phase 4 coverage goals.tests/unit/publish/test_publish_manifest_strategies.py (1)
33-138: Retain the helper and scenario matrix for strip‑patch strategiesKeep
_test_strip_patch_strategy_helperand the surrounding tests as written; they provide a clear, low-duplication matrix over “all”, “per-crate”, missing manifests, invalid TOML, unknown strategies, and non‑crates‑io patches, which is exactly the behaviour surface that_apply_strip_patch_strategyneeds exercised.tests/unit/publish/test_publish_diagnostics.py (1)
14-68: Keep diagnostics tests as writtenLeave these diagnostics tests intact; they correctly probe artefact discovery, missing files, deduplication, tail handling, and the “no content” path, and they match the intended semantics of the helpers in
publish_diagnostics.Also applies to: 84-91
tests/unit/publish/test_publish_execution_helpers.py (1)
75-287: Retain the expanded publish‑execution helper testsKeep the rest of this module as written. The tests exercise the important edge cases for cmd‑mox integration, subprocess handling, environment normalisation and logging, PATH merging, and I/O relaying, all using local stubs and fixtures without hitting real external commands. This aligns well with the PR’s Phase 4 stability and coverage goals.
…ency validation errors Improved test coverage for workspace model validation by parameterizing tests that check invalid dependency mappings and kinds. This change replaces multiple individual test cases with a more maintainable structure using pytest.param and pytest.mark.parametrize. Also added tests to detect workspace readme flags and additional validation error cases. 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 (3)
tests/unit/publish/test_publish_manifest_strategies.py (1)
18-26: Aligntype: ignoreusage with pyright-focused guidelinesReplace the remaining
# type: ignore[arg-type]at Line 99 with a targeted pyright suppression and an inline justification, matching the pattern used forpublishablein_make_plan. For example, use# pyright: ignore[reportArgumentType] - exercising invalid-strategy branch in testsso static analysis remains useful while still permitting this negative-path call in tests.Also applies to: 33-45, 86-100
docs/lading-design.md (2)
525-537: Rewrite the directory structure code block as a proper multi-line treeSplit this plaintext block so each path appears on its own line with the tree glyphs (
│,├──,└──) restored, and move inline comments onto their own indented lines (or keep them after filenames with a single space). The current single-line concatenation makes the tree unreadable and conflicts with the “proposed directory structure” description.
567-578: EnsureIPCis expanded on its first occurrence in the documentKeep the new Phase 4 bullets, but update the first occurrence of “IPC” earlier in the document to read “inter-process communication (IPC)” so the acronym is defined at first use. Once that is in place, keep later mentions (including here) as “IPC” only, or leave a single explicit expansion if style guidance prefers.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (7)
docs/lading-design.md(4 hunks)tests/unit/publish/test_publish_diagnostics.py(1 hunks)tests/unit/publish/test_publish_execution_helpers.py(1 hunks)tests/unit/publish/test_publish_manifest_strategies.py(1 hunks)tests/unit/test_toml_utils.py(1 hunks)tests/unit/test_workspace_metadata.py(2 hunks)tests/unit/test_workspace_models_validation.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/test_workspace_metadata.pytests/unit/test_workspace_models_validation.pytests/unit/publish/test_publish_diagnostics.pytests/unit/publish/test_publish_manifest_strategies.pytests/unit/publish/test_publish_execution_helpers.pytests/unit/test_toml_utils.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/test_workspace_metadata.pytests/unit/test_workspace_models_validation.pytests/unit/publish/test_publish_diagnostics.pytests/unit/publish/test_publish_manifest_strategies.pytests/unit/publish/test_publish_execution_helpers.pytests/unit/test_toml_utils.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/test_workspace_metadata.pytests/unit/test_workspace_models_validation.pytests/unit/publish/test_publish_diagnostics.pytests/unit/publish/test_publish_manifest_strategies.pytests/unit/publish/test_publish_execution_helpers.pytests/unit/test_toml_utils.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/test_workspace_metadata.pytests/unit/test_workspace_models_validation.pytests/unit/publish/test_publish_diagnostics.pytests/unit/publish/test_publish_manifest_strategies.pytests/unit/publish/test_publish_execution_helpers.pytests/unit/test_toml_utils.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/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/lading-design.md
🧬 Code graph analysis (5)
tests/unit/test_workspace_metadata.py (1)
lading/workspace/metadata.py (10)
CargoMetadataError(22-33)_CmdMoxCommand(126-158)run(136-158)_resolve_cmd_mox_timeout(184-194)_build_invocation_environment(161-168)_coerce_text(97-101)invalid_cmd_mox_timeout(26-28)non_positive_cmd_mox_timeout(31-33)_load_cmd_mox_modules(171-181)argv(132-134)
tests/unit/test_workspace_models_validation.py (1)
lading/workspace/models.py (17)
WorkspaceDependency(50-56)_is_ordering_dependency(21-30)WorkspaceModelError(33-34)build_workspace_graph(184-215)_index_workspace_packages(218-230)_build_dependencies(261-281)_validate_dependency_mapping(284-291)_validate_dependency_kind(312-327)_lookup_workspace_target(294-309)_normalise_workspace_root(354-363)_normalise_manifest_path(366-372)_expect_sequence(375-392)_expect_string(395-400)_is_non_empty_sequence(403-409)_coerce_publish_setting(412-425)_extract_readme_workspace_flag(428-436)_manifest_uses_workspace_readme(439-452)
tests/unit/publish/test_publish_diagnostics.py (1)
lading/commands/publish_diagnostics.py (3)
_append_compiletest_diagnostics(56-79)_read_tail_lines(29-38)_format_artifact_diagnostics(41-53)
tests/unit/publish/test_publish_manifest_strategies.py (2)
lading/commands/publish_plan.py (2)
PublishPlan(22-34)publishable_names(32-34)lading/commands/publish_manifest.py (2)
_apply_strip_patch_strategy(182-205)PublishPreparationError(58-74)
tests/unit/test_toml_utils.py (1)
lading/testing/toml_utils.py (7)
load_or_create_document(50-72)ensure_table(75-108)ensure_array_field(111-143)append_if_absent(146-163)load_manifest(166-188)load_workspace_manifest(191-210)load_crate_manifest(213-239)
🪛 GitHub Actions: CI
tests/unit/publish/test_publish_diagnostics.py
[error] 1-1: Command failed: make check-fmt. Ruff format --check would reformat 1 file (tests/unit/publish/test_publish_diagnostics.py).
🔍 Remote MCP Ref
Summary of additional, review-relevant facts
- pyproject.toml on the PR branch adds pytest-cov to the dev dependency group and pins cmd-mox via git URL; pytest timeout set to 30s.
- ruff/pylint max-args is configured to 4 (tool.ruff.lint.pylint -> max-args = 4) — explains the PR comments about reducing test function argument counts.
- The PR is online: Finalize test suite and enhance docs (Phase 4) — PR #45 (branch terragon/phase4-stabilisation-testing-docs-aem45j → main). Use this URL for direct review and CI logs.
- I inspected the repo comparison view for the branch (diff/compare) for context on changed files. Use this to see full diffs per-file when reviewing.
- The project uses pytest; see pytest docs for plugin/pytest-cov integration and invocation guidance if you need to run coverage locally (pytest --cov).
Relevant implications for review
- Tests+docs heavy PR: expect CI to run pytest with coverage; ensure new tests run within the 30s timeout and adhere to lint complexity/arg limits (max-args=4).
- CMD-MOX is included as a test-time dependency (git-pinned) — validate that the CI environment can fetch that git URL.
- Many changes are test additions and doc edits; production code changes are limited to adding pragma comments in TYPE_CHECKING blocks (coverage/annotation-only).
⏰ 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 (4)
tests/unit/test_workspace_metadata.py (1)
394-538: Strengthen cmd‑mox coverage further only if future regressions appearKeep these new cmd‑mox tests as written; they exercise socket absence, timeout parsing, invocation environment, text coercion, error constructors, module‑load failure, and IPC execution paths in a clear, focused way and align with the workspace.metadata contract. No changes are required here.
tests/unit/test_toml_utils.py (1)
16-96: Keep this TOML utilities test suite as-isRetain these tests; they now exercise both failure and success paths for
load_or_create_document,ensure_table,ensure_array_field,append_if_absent, and the manifest loaders, matching the documented behaviour oflading.testing.toml_utils.tests/unit/test_workspace_models_validation.py (1)
15-139: Retain these workspace model validation testsKeep this module as written; it thoroughly exercises the defensive and validation helpers in
lading.workspace.models(including dependency shapes, path/sequence/string coercion, publish settings, and readme workspace flags) and aligns with the intended error contracts.tests/unit/publish/test_publish_execution_helpers.py (1)
17-287: Keep the consolidated cmd‑mox fixture and execution helper testsLeave this module as-is; the
mock_cmd_mox_modulesfixture cleanly encapsulates env/ipc/runner stubs, and the tests collectively exercise all key publish execution paths (subprocess spawning, PATH merging, cmd‑mox passthrough, buffering, broken pipes, environment updates, and logging) without over‑mocking.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
tests/unit/publish/test_publish_diagnostics.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_publish_diagnostics.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_publish_diagnostics.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_publish_diagnostics.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_publish_diagnostics.py
🧬 Code graph analysis (1)
tests/unit/publish/test_publish_diagnostics.py (1)
lading/commands/publish_diagnostics.py (3)
_append_compiletest_diagnostics(56-79)_read_tail_lines(29-38)_format_artifact_diagnostics(41-53)
🪛 GitHub Actions: CI
tests/unit/publish/test_publish_diagnostics.py
[error] 11-11: ruff check failed: F401 'pytest' imported but unused. This is fixable with the '--fix' option.
🔍 Remote MCP Ref
Summary of additional, review-relevant facts found on the PR page (#45):
-
PR metadata: branch terragon/phase4-stabilisation-testing-docs-aem45j → main, 5 commits, +1,110 −37, files changed: 19; title “Finalize test suite and enhance docs (Phase 4)”; opened 2025-12-04.
-
Key intent: add Phase‑4 testing scaffolding and docs, introduce pytest‑cov as dev dependency, add many unit tests (8 new test modules covering publish diagnostics/execution/manifest strategies, config, toml utils, workspace metadata/models, shared helpers), and annotate TYPE_CHECKING blocks with # pragma: no cover (coverage-only, no runtime changes). CI/test invocation recommended: uv run pytest --cov or pytest --cov.
-
Test/CI implications flagged in the PR and review bots:
- pytest timeout set to 30s; ensure new tests complete within that limit.
- ruff/pylint max-args configured to 4 — several review comments requested consolidating fixtures to keep test argument counts ≤4.
- cmd-mox is added/pinned as a git dependency for tests; CI must be able to fetch that URL.
-
Notable reviewer feedback and automated findings (actionable for review):
- Large Method, Code Duplication, and Excess-Args issues were raised in tests and then addressed by introducing fixtures/helpers and consolidating fixtures into a single composite fixture. Commits show refactors (fixtures consolidated, helper added). Validate these refactors preserve test semantics and satisfy lint (max-args) and complexity limits.
- Docs issues: malformed ASCII directory tree in docs/lading-design.md (flattened lines), inconsistent capitalization of “cmd-mox” vs “Cmd-mox”, and missing expansion of “IPC” on first use — reviewers requested fixes. Confirm doc formatting and acronym expansion.
- CI/coverage checklist: reviewers requested updating CI workflow to use pytest-cov (replace slipcover) and measure coverage for both crate_tools and lading packages to substantiate roadmap claim of >90% coverage.
-
Practical review checklist derived from above (verify before approve/merge):
- Run tests with pytest --cov (or uv run pytest --cov) to confirm coverage and that Phase‑4 >90% targets are met for new modules.
- Ensure tests complete under the 30s timeout and adhere to lint rules (max-args=4, complexity thresholds).
- Confirm cmd-mox dependency is resolvable in CI (git URL) and that introduced stubs/fixtures accurately exercise intended behavior without network/IPC flakiness.
- Verify requested doc fixes (directory tree formatting, acronym expansion, capitalization) are applied.
- Confirm TYPE_CHECKING # pragma: no cover additions are non-functional and intended only to affect coverage.
Sources:
- Pull request page and conversation for PR #45 (Finalize test suite and enhance docs (Phase 4)) — GitHub PR view.
⏰ 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 (5)
tests/unit/publish/test_publish_diagnostics.py (5)
14-28: LGTM!The test correctly verifies that artifact tail lines are appended to diagnostics when artifacts exist.
31-44: LGTM!The test properly verifies that missing artifacts are handled gracefully without raising exceptions.
47-53: LGTM!The test correctly verifies that the original message is returned unchanged when no artifacts are detected.
56-66: LGTM!The test correctly validates that duplicate artifact tokens are deduplicated in the diagnostic output.
82-89: LGTM!The test correctly verifies that artifacts without content still produce formatted output with the path listed.
…ith monkeypatch Refactored test_read_tail_lines_handles_zero_and_errors to use pytest monkeypatch fixtures for simulating OSError in Path.read_text. This improves test reliability and clarity in handling error cases. Also formatted docs/lading-design.md content for improved readability. Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>
|
@coderabbitai review |
leynos
changed the title
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_publish_infrastructure.py Comment on lines +245 to +254 def _cmd_mox_stub_env_enabled() -> typ.Iterator[None]:
"""Temporarily enable CMD_MOX_STUB_ENV_VAR for cmd-mox stubs."""
var_name = metadata_module.CMD_MOX_STUB_ENV_VAR
snapshot = {var_name: os.environ.get(var_name)}
os.environ[var_name] = "1"
try:
yield
finally:
os.environ.pop(var_name, None)
os.environ.update({k: v for k, v in snapshot.items() if v is not None})❌ New issue: Bumpy Road Ahead |
Improved wording in the lading-design documentation to better describe the focus areas of the test coverage, including configuration validation edges, publish manifest handling, cmd-mox IPC fallback mechanisms, and workspace model error paths. This enhances readability and precision in the docs related to coverage and testing strategies. Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>
This comment was marked as resolved.
This comment was marked as resolved.
leynos
changed the title
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
…stub helper Refactored _cmd_mox_stub_env_enabled helper to handle restoring environment variable more cleanly by conditionally updating with a restore dictionary instead of always using a snapshot dict. This simplifies and clarifies environment cleanup after 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.
|
@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_publish_infrastructure.py Comment on lines +245 to +255 def _cmd_mox_stub_env_enabled() -> typ.Iterator[None]:
"""Temporarily enable CMD_MOX_STUB_ENV_VAR for cmd-mox stubs."""
var_name = metadata_module.CMD_MOX_STUB_ENV_VAR
previous = os.environ.get(var_name)
restore = {var_name: previous} if previous is not None else {}
os.environ[var_name] = "1"
try:
yield
finally:
os.environ.pop(var_name, None)
os.environ.update(restore)❌ New issue: Bumpy Road Ahead |
This comment was marked as resolved.
This comment was marked as resolved.
…lish_infrastructure.py Introduce _build_env_restore_dict helper to consolidate environment variable restoration logic in _cmd_mox_stub_env_enabled context manager, improving code readability and reuse. Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>
leynos
changed the title
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_publish_infrastructure.py Comment on lines +244 to +247 def _build_env_restore_dict(var_name: str) -> dict[str, str]:
"""Build a dictionary for restoring an environment variable."""
previous = os.environ.get(var_name)
return {var_name: previous} if previous is not None else {}❌ New issue: Deep, Nested Complexity |
|
@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_publish_infrastructure.py Comment on lines +244 to +247 def _build_env_restore_dict(var_name: str) -> dict[str, str]:
"""Build a dictionary for restoring an environment variable."""
previous = os.environ.get(var_name)
return {var_name: previous} if previous is not None else {}❌ New issue: Bumpy Road Ahead |
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
Refactored the _build_env_restore_dict function to improve clarity by replacing the ternary operation with a standard if-statement that returns an empty dictionary if the variable is not set, enhancing readability in test utilities. 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: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
tests/bdd/steps/test_publish_infrastructure.py(6 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/bdd/steps/test_publish_infrastructure.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_publish_infrastructure.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_publish_infrastructure.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_publish_infrastructure.py
🧬 Code graph analysis (1)
tests/bdd/steps/test_publish_infrastructure.py (1)
tests/bdd/steps/test_publish_fixtures.py (2)
preflight_overrides(15-17)preflight_recorder(21-23)
🔍 Remote MCP Deepwiki, Ref
Relevant review facts (concise)
- Test tooling: pytest-cov was added to dev dependencies; PR expects running tests with coverage (target >90%).,
- Cmd-mox test stubbing: repository uses cmd-mox IPC to stub cargo/git in BDD tests; tests enable it via LADING_USE_CMD_MOX_STUB and require CMOX_IPC_SOCKET (and optional CMOX_IPC_TIMEOUT). CI must provide/install cmd-mox and set socket for BDD runs.
- publish behavior change: allow_dirty flag now flows via a new internal _PublishExecutionOptions and is propagated into packaging and cargo publish/package invocations (adds --allow-dirty when set). Call sites updated accordingly. Verify all call sites updated and tests reflect expected command args.
- Signature/API changes to review: _package_publishable_crates and _publish_crates now accept options: _PublishExecutionOptions (internal dataclass) — ensure no remaining usages still using old params.
- New/expanded tests: many unit and BDD tests added (publish diagnostics, execution helpers, manifest strategies, config, toml utils, workspace model/metadata). They rely on cmd-mox fixtures and on configuration/context patterns — ensure fixtures/autouse preflight stubbing and test scaffolding are correct.
- Docs/usage: publish workflow, strip_patches semantics (all / per-crate / false), staging, and README propagation were updated — verify docs match implemented behavior/tests.
- CI checklist for this PR:
- Install pytest-cov and run pytest --cov to validate >90% for Phase 4 modules.
- Ensure cmd-mox is available and CMOX_IPC_SOCKET is set for BDD runs.
- Run linters/typechecks because small TYPE_CHECKING pragma changes were added across modules.
- Confirm new internal dataclass _PublishExecutionOptions is not exported unintentionally.
🔇 Additional comments (2)
tests/bdd/steps/test_publish_infrastructure.py (2)
59-67: Keep allow_dirty wiring but verify callers of_create_stub_configuse keyword arguments.Threading
allow_dirtythrough_PreflightStubConfig,PreflightTestContext.create_stub_config, and_create_stub_configis coherent and matches the Phase 4 publish semantics._create_stub_configis now keyword‑only forallow_dirty, so any direct call sites that previously passed it positionally will now fail.Run a quick search and update any direct
_create_stub_configusages to passallow_dirtyby name if they exist outside this file.#!/bin/bash # Verify all _create_stub_config call sites use keyword arguments for allow_dirty. rg "_create_stub_config" -nAlso applies to: 77-81, 158-171
293-297: Approve additional coverage for cargo publish normalisation.Extend the parametrised test with the
("cargo", "publish", "--allow-dirty", "--dry-run")case and assert the normalised program/args tuple exactly matches the new_normalise_cmd_mox_commandbehaviour. This gives direct coverage for theallow_dirty+--dry-runpath and looks correct.
|
@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_publish_infrastructure.py Comment on lines +244 to +249 def _build_env_restore_dict(var_name: str) -> dict[str, str]:
"""Build a dictionary for restoring an environment variable."""
previous = os.environ.get(var_name)
if previous is None:
return {}
return {var_name: previous}❌ New issue: Deep, Nested Complexity |
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Avoid processing more than one cargo publish command in preflight checks to ensure correct test behavior. Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Gates Passed
See analysis details in CodeScene
Absence of Expected Change Pattern
- lading/lading/commands/publish.py is usually changed with: lading/tests/bdd/steps/test_publish_steps.py
Quality Gate Profile: Pay Down Tech Debt
Want more control? Customize Code Health rules or catch issues early with our IDE extension and CLI tool.
1 participant
Summary
Changes
Test plan
Why this matters
Phase 4 stabilisation relies on high-coverage, deterministic tests around configuration validation, publish manifest handling, and workspace model behavior. The added tests, tooling, and documentation provide a solid baseline for ongoing development and future improvements, while enabling visible coverage metrics and clearer expectations for operators.
📎 Task: https://www.terragonlabs.com/task/98bbccf3-4b8c-4c07-ba21-8d2555e8840f
📎 Task: https://www.terragonlabs.com/task/1210b6b1-1745-48d0-86a6-a9f821c02519