Skip to content

fix: codebase review cleanup#55

Merged
markhedleyjones merged 7 commits intomainfrom
fix/codebase-review-cleanup
Mar 26, 2026
Merged

fix: codebase review cleanup#55
markhedleyjones merged 7 commits intomainfrom
fix/codebase-review-cleanup

Conversation

@markhedleyjones
Copy link
Copy Markdown
Owner

Addresses findings from full codebase review:

  • Guard against empty or non-mapping cm.yaml in from_yaml
  • Read version from package metadata instead of hardcoding 0.1.0
  • Add trap for manifest temp file cleanup in run.sh
  • Redirect runtime detection error to stderr in run.sh
  • Fix American English spellings in cli help text
  • Fix cached-assets docs (cm build downloads, not cm update)
  • Remove dead _macros.j2 template and unused asset dest reference
  • Extract shared test helpers into unit conftest
  • Deduplicate feature flags and remove unnecessary runtime guards
  • Add unit tests for run_container argument assembly

…on exit

- Guard against empty or non-mapping cm.yaml in from_yaml (crashes with
  confusing TypeError)
- Read __version__ from package metadata instead of hardcoding 0.1.0
  (was never updated by semantic-release)
- Add trap for manifest temp file cleanup in run.sh command functions
  (leaked on SIGINT or container failure)
- Redirect runtime detection error to stderr in run.sh
- "Initialize"/"Initializing" to "Initialise"/"Initialising" in cli
  help text and messages
- cached-assets.md incorrectly said cm update downloads assets; only
  cm build does
- _macros.j2 defined a symlink_mounts macro never imported by any
  template
- asset.get("dest") in cache list command referenced a key that
  list_cached_assets never sets
Move duplicated _generate (generate_dockerfile_from_dict) and
_get_stage_block (get_stage_block) helpers into tests/unit/conftest.py.
Previously defined independently in test_pip_venv.py,
test_implicit_user.py, test_dockerfile_output.py, and test_assets.py.
…ards

- Rename _build_feature_flags to build_feature_flags and import it in
  run_script.py instead of duplicating the dict construction
- Remove "if config.runtime else" guards throughout run_script.py and
  runner.py since RuntimeConfig always has a default_factory
14 tests covering the main code paths: basic args, workspace mount,
workdir, image not found, ipc/network/privileged modes, volume
expansion with SELinux labels, device passthrough, ad-hoc exec form,
custom command shell wrapping with env vars and ports, and detach mode.
@markhedleyjones markhedleyjones force-pushed the fix/codebase-review-cleanup branch from 9e9a7b2 to a0517e1 Compare March 26, 2026 09:27
@markhedleyjones markhedleyjones merged commit c8da2bf into main Mar 26, 2026
8 checks passed
@markhedleyjones markhedleyjones deleted the fix/codebase-review-cleanup branch March 26, 2026 09:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant