Remove backwards-compat shims guarding private symbols (#163)

[lodyai]

Summary

This branch sweeps lading/ for backwards-compatibility shims, aliases, and wrappers guarding private (underscore-prefixed) symbols and removes those serving no genuine external-compatibility need. Private symbols carry no API stability contract, and lading's application code is the only consumer of its own internals, so the ad hoc compatibility layers kept solely so old test patches would keep resolving have been deleted, with the dependent tests retargeted at the canonical implementations.

Closes #163.

Inventory and disposition

Location	Shim	Disposition
lading/commands/publish.py	Eleven bare aliases of publish_preflight privates (_preflight_argument_sets, _CargoPreflightOptions, _run_cargo_preflight, and friends)	Removed; tests patch and call publish_preflight directly
lading/commands/publish.py	_run_preflight_checks thin wrapper preserving the optional-configuration contract	Removed; run() resolves configuration itself and calls publish_preflight._run_preflight_checks directly
lading/commands/publish.py	Unused re-bindings _append_section, _format_plan, metadata_module, StripPatchesSetting, PublishPlanError	Removed; plan-validation tests use publish_plan.PublishPlanError
lading/commands/bump.py	Six re-exports of bump_toml helpers kept "to maintain backward compatibility", plus the redundant _log = LOGGER alias	Removed; TOML helper tests exercise bump_toml directly
lading/commands/publish_plan.py	Private definitions _append_section/_format_plan re-bound to the public names every consumer used	Definitions renamed to the public names; aliases and stale __all__ entries dropped
lading/commands/publish_execution.py	split_command wrapper (plus _split_command) over the runtime helper, with no production callers	Removed along with its only test; cmd_mox_runner already uses lading.runtime.subprocess_runner.split_command
lading/commands/publish.py _invoke import	Retained	Genuine dependency-injection seam: run() uses it as the default CommandRunner, and tests stub it to intercept subprocess execution

No new port/adapter seam proved necessary: the churn that motivated the shims was a one-off module extraction (#96/#123), and the existing CommandRunner protocol in lading.runtime already provides the stable boundary for execution concerns.

Review walkthrough

• Start with lading/commands/publish.py to see the alias block and wrapper removed and run() calling publish_preflight._run_preflight_checks directly.
• Then review tests/unit/publish/conftest.py and tests/unit/publish/test_preflight_checks.py for the retargeted patch points; the two tests that pinned the wrapper's aliasing behaviour are removed with the wrapper.
• lading/commands/publish_plan.py and lading/commands/publish_execution.py show the canonical-name promotion and dead wrapper removal.
• lading/commands/bump.py with tests/unit/test_bump_toml_helpers.py covers the bump alias removal.
• Finish with docs/developers-guide.md for the new "Internal APIs carry no compatibility aliases" convention and the rewritten pre-flight section, and docs/lading-design.md for the corrected publish-flow diagram.

Validation

• make check-fmt: passed (137 files already formatted)
• make lint: passed (ruff clean, interrogate 100%, pylint 10.00/10)
• make typecheck: passed (ty check, all checks passed)
• make test: passed (691 passed, 67 snapshots passed)
• make markdownlint: passed (21 files, 0 errors)
• make nixie: passed (all diagrams validated)
• coderabbit review --agent: completed with 0 findings

References

• Issue: Sweep codebase for unnecessary backwards-compatibility shims on private APIs #163
• Prior work: De-duplicate _run_preflight_checks / _preflight_argument_sets to a single home #96, De-duplicate preflight helpers into publish_preflight (#96) #123
• Lody session: https://lody.ai/leynos/sessions/135fbb00-abc8-41b5-b7c2-1e9a0f9bd51c

[sourcery-ai]

Sorry @LodyAI[bot], you have reached your weekly rate limit of 500000 diff characters.

Please try again later or upgrade to continue using Sourcery

[coderabbitai]

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

Walkthrough

Remove private compatibility aliases and wrappers, route publish pre-flight calls and tests to canonical modules, export publish-plan helpers, standardize bump logging, migrate TOML helper usage, and update developer documentation.

Changes

Shim removal and canonical-target migration

Layer / File(s)	Summary
Documentation and design updates docs/developers-guide.md, docs/lading-design.md, typos.toml	Document private API stability, direct pre-flight integration, the updated publish execution flow, and spelling mappings.
Publish pre-flight direct invocation lading/commands/publish.py	Remove the pre-flight wrapper and aliases, import publish_preflight directly, and preserve the direct PublishPlanError re-export.
Publish pre-flight test retargeting tests/unit/conftest.py, tests/unit/publish/*	Patch and invoke pre-flight helpers, option types, and exceptions from publish_preflight.
Publish execution helper cleanup lading/commands/publish_execution.py, tests/unit/publish/test_command_helpers.py	Remove split_command and test runtime subprocess and cmd-mox helpers directly.
Publish plan public helper renaming lading/commands/publish_plan.py, tests/unit/publish/*, tests/unit/test_publish_*	Rename and export plan-formatting helpers, update references, and validate the canonical PublishPlanError type.
Bump logging and TOML helper migration lading/commands/bump.py, tests/unit/test_bump_*.py	Use LOGGER directly, add bump TOML helper aliases, and migrate tests to bump_toml APIs.

Possibly related issues

• leynos/lading#170: Both changes update compatibility boundaries and retarget related test seams.

Possibly related PRs

• leynos/crate-tools#20: Both changes modify the publish run() orchestration path.
• leynos/lading#21: Both changes alter publish pre-flight invocation and helper boundaries.
• leynos/lading#135: Both changes overlap on the publish_preflight execution path.

Suggested labels: Issue

Suggested reviewers: leynos, codescene-delta-analysis

Poem

Private shims fade from view,
Canonical calls now travel true.
Plans format with names made bright,
Pre-flight tests patch the source just right.
LOGGER hums through TOML’s tune.

---

Caution

Pre-merge checks failed

Please resolve all errors before merging. Addressing warnings is optional.

• Ignore

❌ Failed checks (1 error, 2 warnings)

Check name	Status	Explanation	Resolution
Unit Architecture	❌ Error	lading/commands/bump.py still re-exports private bump_toml helpers for compatibility, yet tests now call bump_toml directly and no legacy bump._... uses remain.	Remove the leftover alias block from bump.py and keep callers/tests on lading.commands.bump_toml so the internal boundary stays explicit.
Linked Issues check	⚠️ Warning	Issue #163 is not fully met because bump.py still adds backwards-compatibility re-exports for helper functions instead of removing them.	Remove the new bump.py compatibility aliases and keep callers and tests on bump_toml directly.
Out of Scope Changes check	⚠️ Warning	The PR includes unrelated typos.toml spelling-map edits that are not required by issue #163.	Move the typos.toml updates into a separate change or justify them within the issue scope.

✅ Passed checks (17 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly states the shim-removal focus and includes the linked issue reference.
Description check	✅ Passed	The description matches the shim-removal work and stays on-topic for the code changes.
Docstring Coverage	✅ Passed	Docstring coverage is 86.96% which is sufficient. The required threshold is 80.00%.
Testing (Overall)	✅ Passed	Tests assert filesystem writes, command arguments, error text, and public re-exports; no vacuous mock-only coverage found.
User-Facing Documentation	✅ Passed	Pass: leave docs/users-guide.md untouched; the PR only removes internal shims and helper aliases, with no CLI behaviour change.
Developer Documentation	✅ Passed	Verify docs/developers-guide.md and docs/lading-design.md were updated with the new internal-API boundaries; no execplan file is present.
Module-Level Documentation	✅ Passed	All changed Python modules carry top-level docstrings; the publish, bump, plan, execution, and test modules all describe purpose and component relationships.
Testing (Unit And Behavioural)	✅ Passed	PASS: The patch adds meaningful unit coverage for edge cases/error paths and preserves boundary coverage via publish.run/bump.run plus existing BDD/e2e CLI tests.
Testing (Property / Proof)	✅ Passed	Treat this as a shim-removal refactor: it introduces no new range-wide invariant or proof assumption, so property tests or proofs are not warranted.
Testing (Compile-Time / Ui)	✅ Passed	No Rust/TS compile-time surface exists in the touched code, and the human-readable publish summary is already guarded by focused snapshot tests in test_run_workspace_config.
Domain Architecture	✅ Passed	Only docs and the command-layer publish_plan module changed; no domain model was touched, and publish_plan still translates workspace/config concepts at the application boundary.
Observability	✅ Passed	No new operational path lacks logs/metrics; publish, preflight, subprocess, and lockfile flows already log failures, state, and redacted commands.
Security And Privacy	✅ Passed	Only internal shims/docs/tests were retargeted; scans found no secrets, auth changes, unsafe deserialisation, or shell/eval injection in the touched code.
Performance And Resource Use	✅ Passed	Only wrapper/alias removals and a linear section formatter changed; no new unbounded loops, repeated I/O, or material allocations appeared.
Concurrency And State	✅ Passed	The PR only retargets shims/tests and docs; publish and preflight remain synchronous, and shared pipeline state stays in frozen dataclasses.
Architectural Complexity And Maintainability	✅ Passed	PASS: the PR removes shims and thin wrappers, keeps only documented seams like _invoke, and introduces no new layers, registries, or orchestration machinery.
Rust Compiler Lint Integrity	✅ Passed	No Rust files or Rust lint suppressions changed; the diff only touches docs and Python, so the check is not applicable.

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch issue-163-sweep-codebase-for-unnecessary-backwards-compat-shims

---

_{Comment @coderabbitai help to get the list of available commands.}

[sourcery-ai]

Sorry @leynos, you have reached your weekly rate limit of 500000 diff characters.

Please try again later or upgrade to continue using Sourcery

[pandalump]

@coderabbitai resume

[coderabbitai]

✅ Action performed

Reviews resumed.

[leynos]

@coderabbitai Have the following now been resolved?

lading/commands/bump.py (1)

44-44: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Remove the private TOML compatibility aliases.
The cleanup at Line 44 is undermined by the aliases added at Lines 542-543. Remove that compatibility block; it recreates the undocumented private API that Issue #163 explicitly requires this PR to eliminate. Keep callers and tests on the canonical lading.commands.bump_toml helpers.
Also applies to: 542-543

🤖 Detailed instructions

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In @lading/commands/bump.py at line 44, Remove the private TOML compatibility
alias block near the module’s TOML helper definitions, including the aliases
around lines 542-543. Update any remaining callers or tests to use the canonical
lading.commands.bump_toml helpers, leaving the existing cleanup intact.

lading/commands/publish_plan.py (1)

215-225: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Document the public helpers and add the explicit return.
Add NumPy-style Parameters and Returns sections to append_section and format_plan, then finish append_section with a bare return instead of relying on implicit None.

def append_section[T](...):
     ...
         lines.extend(f"- {formatter(item)}" for item in items)
+    return

As per coding guidelines and path instructions, public interfaces require NumPy-format docstrings and explicit returns.
Also applies to: 228-229

🤖 Detailed instructions

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In @lading/commands/publish_plan.py around lines 215 - 225, Add NumPy-style
Parameters and Returns sections to the public helpers append_section and
format_plan, documenting their arguments and None return values. In
append_section, add an explicit bare return after the conditional
section-appending logic while preserving its existing behavior.

Sources: Coding guidelines, Path instructions

[leynos]

@coderabbitai Have the following now been resolved?

lading/commands/bump.py (1)

542-543: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Remove the private compatibility re-exports.
Delete the bottom-of-file aliases at Lines 542-543. The migrated tests already call bump_toml.*, so retaining underscore-prefixed names on bump recreates the private API shims that this PR and Issue #163 explicitly require removing.

🤖 Detailed instructions

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In @lading/commands/bump.py around lines 542 - 543, Remove the bottom-of-file
private compatibility aliases following the return in the relevant bump module,
leaving only the sections return and no underscore-prefixed re-exports on bump.
Preserve the migrated bump_toml API usage and avoid adding replacement shims.

lading/commands/publish_plan.py (1)

215-223: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Document the newly exported helpers as public interfaces.
Add NumPy-style Parameters and Returns sections to append_section and format_plan. Removing the private marker and adding both names to __all__ makes the current one-line summaries insufficient.
Also applies to: 228-229, 262-267

🤖 Detailed instructions

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In @lading/commands/publish_plan.py around lines 215 - 223, The exported helpers
append_section and format_plan need complete public API documentation. Remove
their private markers, add both function names to all, and expand each
docstring with NumPy-style Parameters and Returns sections describing their
arguments and return behavior while preserving the existing summaries.

Sources: Coding guidelines, Path instructions

[leynos]

@coderabbitai the rules prohibit implicit return only if the function may return a value.

R501 — Avoid Explicit return None if It's the Only Return

# BAD:
def func():
    return None

# GOOD:
def func():
    return

Use return alone instead of return None when the function's only result is None.

---

R502 — Avoid Implicit None in Functions That May Return a Value

# BAD:
def func(x):
    if x > 0:
        return x
    # implicitly returns None (bad)

# GOOD:
def func(x):
    if x > 0:
        return x
    return 0

Ensure all branches explicitly return a value if any branch does.

---

R503 — Add an Explicit Return at the End

# BAD:
def func(x):
    if x > 0:
        return x
    # no return (bad)

# GOOD:
def func(x):
    if x > 0:
        return x
    return -1

Don't rely on implicit None—always return something at the end.

This is unclear. Please raise GitHub issues against leynos/agent-template-python and leynos/lading recommending:

Don't rely on implicit None if the function may return a value elsewhere—always return something at the end.

[leynos]

@coderabbitai resume

[coderabbitai]

✅ Action performed

Reviews resumed.

[coderabbitai]

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@docs/developers-guide.md`:
- Around line 544-554: Add a concise caption immediately before the shim
inventory table in the developers guide, using wording such as “Shim inventory.”
Preserve the table content and formatting unchanged.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: edff1c1d-662e-49a2-abdf-8534b66194cc

📥 Commits

Reviewing files that changed from the base of the PR and between e178994 and 66f99ac.

📒 Files selected for processing (4)

• docs/developers-guide.md
• lading/commands/publish_plan.py
• tests/unit/test_publish_planning.py
• typos.toml

🔗 Linked repositories identified

CodeRabbit considers these linked repositories for cross-repo context during reviews:

• leynos/cmd-mox (auto-detected)
• leynos/cuprum (auto-detected)
• leynos/shared-actions (auto-detected)

[coderabbitai]

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@lading/commands/publish_plan.py`:
- Around line 274-284: Update the PublishPlan doctest in format_plan to
construct workspace_root with Path("ws") instead of an absolute Path("/ws"), and
change the expected “Publish plan for” output to “ws” so the example is
platform-neutral.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 7981516c-30cc-4d20-bdfd-2c7b1bbfb30d

📥 Commits

Reviewing files that changed from the base of the PR and between 66f99ac and 1c320e2.

📒 Files selected for processing (1)

• lading/commands/publish_plan.py

🔗 Linked repositories identified

CodeRabbit considers these linked repositories for cross-repo context during reviews:

• leynos/cmd-mox (auto-detected)
• leynos/cuprum (auto-detected)
• leynos/shared-actions (auto-detected)

[codescene-access]

Our agent can fix these. Install it.

Gates Passed
6 Quality Gates Passed

Absence of Expected Change Pattern

• lading/lading/commands/bump.py is usually changed with: lading/tests/unit/test_cli.py
• lading/lading/commands/publish.py is usually changed with: lading/tests/bdd/steps/test_publish_steps.py

Quality Gate Profile: Pay Down Tech Debt
Install CodeScene MCP: safeguard and uplift AI-generated code. Catch issues early with our IDE extension and CLI tool.

[codescene-access]

No quality gates enabled for this code.

[pandalump]

@coderabbitai review

[coderabbitai]

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[leynos]

@coderabbitai Have the following now been resolved?

lading/commands/bump.py (1)

44-44: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
Delete the reintroduced private aliases before removing this import.
Line 44 removes bump_toml, but Lines 542-543 still dereference it to create compatibility aliases. Importing lading.commands.bump therefore raises NameError. Delete the bottom compatibility block; the migrated tests already use canonical bump_toml helpers.
Based on the PR objective to remove private compatibility shims, retain no test-only aliases in lading/commands/bump.py.
Also applies to: 542-543

🤖 Detailed instructions

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In @lading/commands/bump.py at line 44, Remove the compatibility alias block
near the bottom of the module that dereferences the removed private import,
including the aliases around bump_toml. Ensure importing the module no longer
references undefined symbols and retain only the canonical helpers without
test-only aliases.

[coderabbitai]

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

printf '%s\n' '== bump.py import and compatibility-alias regions =='
sed -n '30,58p' lading/commands/bump.py
tail -n 80 lading/commands/bump.py

printf '%s\n' '== Any former private TOML alias assignments in bump.py =='
rg -n -C 3 \
  '^\s*_(parse_manifest|select_table|assign_version|value_matches|update_dependency_sections|update_dependency_table)\s*=' \
  lading/commands/bump.py || true

printf '%s\n' '== Current bump_toml import/references in bump.py =='
rg -n -C 2 '\bbump_toml\b' lading/commands/bump.py || true

printf '%s\n' '== Production/test callers of the former bump private helpers =='
rg -n -C 2 \
  '\bbump\._(parse_manifest|select_table|assign_version|value_matches|update_dependency_sections|update_dependency_table)\b|from\s+lading\.commands\.bump\s+import\s+.*_(parse_manifest|select_table|assign_version|value_matches|update_dependency_sections|update_dependency_table)\b' \
  lading tests || true

printf '%s\n' '== PR-head diff for bump-related files =='
git status --short
git diff -- lading/commands/bump.py lading/commands/bump_toml.py tests/unit/test_bump_toml_helpers.py tests/unit/test_bump_manifest_writing.py
git diff --cached -- lading/commands/bump.py lading/commands/bump_toml.py tests/unit/test_bump_toml_helpers.py tests/unit/test_bump_manifest_writing.py

Length of output: 5078

---

@leynos Mark this finding resolved.

lading/commands/bump.py contains neither a bump_toml import/reference nor the former underscore-prefixed TOML compatibility aliases. No production or test caller targets bump._parse_manifest, bump._select_table, bump._assign_version, bump._value_matches, bump._update_dependency_sections, or bump._update_dependency_table.

The reported import-time NameError cannot occur, and the PR retains no test-only private TOML shim.