feat: migrate build/release tooling to uv + python-semantic-release - #67
feat: migrate build/release tooling to uv + python-semantic-release#67tomtranjr wants to merge 7 commits into
Conversation
Replace Poetry + run-number versioning with the uv build backend and python-semantic-release for Conventional-Commit-driven releases. - pyproject.toml: switch build-system to uv_build; set module-name to "deployml" (dist is deployml-core); exclude local .terraform artifacts from the sdist/wheel; add dev group (pytest, python-semantic-release), pytest config, and semantic_release config (build via uv, release on main) - baseline version set to 0.0.60 to match the latest tag/PyPI release; the first automated release bumps to 0.1.0 - .github/workflows/release.yml: uv-based test gate + semantic-release version/publish with OIDC trusted publishing to PyPI (replaces test-pypi.yml) - delete poetry.toml; track uv.lock instead of ignoring poetry.lock - add tests/test_smoke.py (main has no unit tests yet; the suite lives on dev) - docs/contributing.md: document uv workflow and commit-message conventions Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
lokeshmuvva
left a comment
lokeshmuvva
left a comment
There was a problem hiding this comment.
Started reviewing this against the current uv and python-semantic-release docs. Leaving one question now on CI trigger scope; more to follow separately.
One thing not tied to a specific line: this PR targets main, but contributions here normally branch off dev and PR into dev (dev is ahead of main until a promotion). Was targeting main directly intentional for this migration, or should it be rebased onto dev?
| - 'docs/**' | ||
| - '*.md' | ||
| pull_request: | ||
| branches: [ main ] |
There was a problem hiding this comment.
Question: both triggers here (push and pull_request) are scoped to branches: [ main ] only. Is folding dev in (e.g. branches: [main, dev] on the pull_request trigger) out of scope for this migration, or intentionally deferred? As-is, PRs into dev still get zero CI from this workflow — same gap the old test-pypi.yml had.
…mantic-release # Conflicts: # pyproject.toml
test_smoke.py claimed the unit suite still lived on dev and wasn't merged to main; dev landed on main in PR #66, so pytest already collects the full 65-test suite via testpaths, not just these two. Also bound python-semantic-release to <11 to match the pinning discipline used for uv_build, since only a push to main exercises its release CLI path.
|
@coderabbitai full review |
✅ Action performedFull review finished. |
📝 WalkthroughWalkthroughChangesRelease tooling
Runtime code and examples
Validation
Estimated code review effort: 4 (Complex) | ~45 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Nitpick comments (6)
src/deployml/cli/cli.py (5)
175-175: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the redundant local
import json.Line 16 already imports
jsonat module level. The local imports at Line 175 and Line 490 shadow it without effect.♻️ Proposed cleanup
Extract resource details from Terraform outputs and state. Returns a manifest dictionary with all resources that need to be deleted. """ - import json - manifest = {Apply the same removal at Line 490 in
upload_resource_manifest.🤖 Prompt for 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. In `@src/deployml/cli/cli.py` at line 175, Remove the redundant local json imports from the functions around the reported locations, including upload_resource_manifest, and rely on the existing module-level import at line 16.
206-219: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the dead
passstatement and the redundant type check.Line 212 is a no-op inside the loop, and the comment above it already explains that URL parsing is skipped. Line 216 re-tests
output_valtruthiness that Line 215 already required.♻️ Proposed cleanup
for key, value in outputs.items(): output_val = value.get("value", "") - # Cloud Run services - we'll extract from Terraform state instead of URLs - # (URLs contain hash suffixes that don't match actual service names) - pass # Skip URL parsing, will get from state below - - # Storage buckets + # Cloud Run service names come from Terraform state below, not from + # output URLs, because URLs carry hash suffixes. if "_bucket" in key and output_val: - if isinstance(output_val, str) and output_val: + if isinstance(output_val, str): manifest["resources"]["storage_buckets"].append( {"name": output_val} )🤖 Prompt for 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. In `@src/deployml/cli/cli.py` around lines 206 - 219, Remove the no-op pass from the outputs loop and simplify the storage bucket condition by eliminating the redundant isinstance/string truthiness check after output_val has already been validated. Preserve the existing bucket append behavior for keys containing "_bucket" with non-empty output values.
2369-2372: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueImport
timezoneonce at module level.
from datetime import timezoneis repeated as a local import at Lines 1275, 1501, 2263, 2369, and 2486, with two different aliases. Line 17 already importsdatetimeandtimedeltafrom the same module.♻️ Proposed cleanup
-from datetime import datetime, timedelta +from datetime import datetime, timedelta, timezoneThen delete each local
from datetime import timezone/from datetime import timezone as _tzstatement and usetimezone.utcdirectly.🤖 Prompt for 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. In `@src/deployml/cli/cli.py` around lines 2369 - 2372, Move the timezone import to the existing module-level datetime imports, then remove all local timezone and timezone-as-_tz imports in the affected CLI code. Update those call sites to use timezone.utc directly, including the teardown_at calculation near now and duration_hours, while preserving existing behavior.
486-532: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the bucket-resolution logic into one helper.
upload_resource_manifestrepeats theterraform state listplusterraform state showsequence fromupload_terraform_files_to_gcs(Lines 80-127). The two copies must stay in sync, and both use the same loose"name" in line and "=" in linematch. Extract a single_resolve_teardown_bucket(terraform_dir) -> Optional[str]helper and call it from both functions.🤖 Prompt for 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. In `@src/deployml/cli/cli.py` around lines 486 - 532, The Terraform bucket lookup is duplicated between upload_resource_manifest and upload_terraform_files_to_gcs. Extract the shared state-list/state-show parsing into a _resolve_teardown_bucket(terraform_dir) -> Optional[str] helper, including the existing error handling and bucket-name extraction, then replace both functions’ inline logic with calls to that helper.
2192-2243: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftReuse
deployml.api.get_teardown_statusinstead of repeating the query.
show_teardown_statusduplicates the gcloudscheduler jobs describecall and thescheduleTime/lastAttemptTimeparsing thatget_teardown_statusalready performs insrc/deployml/api.py(Lines 45-124). The same duplication exists betweenupdate_teardown_schedulehere andsrc/deployml/api.pyLines 127-243.Call the API functions and keep this command responsible for presentation only. One parsing implementation then serves both the CLI and the notebook API.
🤖 Prompt for 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. In `@src/deployml/cli/cli.py` around lines 2192 - 2243, Replace the duplicated Cloud Scheduler query and parsing in show_teardown_status with deployml.api.get_teardown_status, using its returned status data for CLI output while preserving the existing fallback presentation. Apply the same delegation to update_teardown_schedule by calling the corresponding API function and keeping that command focused on presentation; remove direct gcloud invocation and scheduleTime/lastAttemptTime parsing from both CLI functions.src/deployml/utils/teardown.py (1)
31-44: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove
calculate_teardown_schedule. No callers remain in the repository.🤖 Prompt for 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. In `@src/deployml/utils/teardown.py` around lines 31 - 44, Remove the unused calculate_teardown_schedule function from the teardown utilities module, including its associated documentation and implementation; leave all remaining teardown functionality unchanged.
🤖 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 @.github/workflows/release.yml:
- Line 3: Add workflow-level permissions granting only contents: read for the
lint and test jobs in the release workflow. Preserve the existing release-job
permissions override so publishing and release commits retain their required
access.
- Line 20: Update the actions/checkout@v4 steps at .github/workflows/release.yml
lines 20-20 and 41-41 to set persist-credentials to false for both pull-request
jobs. Leave the release job’s checkout configuration unchanged so
semantic-release can continue pushing commits and tags.
- Around line 28-29: Update all three dependency-sync steps in
.github/workflows/release.yml at lines 28-29, 49-50, and 76-77 to run the locked
sync mode so CI rejects a stale uv.lock. Leave the semantic-release build
command’s uv lock --upgrade-package "$PACKAGE_NAME" invocation unchanged because
it intentionally updates the lockfile.
- Around line 52-53: Update the release workflow’s “Run tests” step to build the
wheel, install that wheel in a clean environment, and execute
tests/test_smoke.py against the installed artifact before publishing. Do not
rely on the editable installation created by uv sync.
In `@src/deployml/cli/cli.py`:
- Around line 265-293: Update extract_resource_manifest in
src/deployml/cli/cli.py at lines 265-293 to parse the attribute key exactly
instead of filtering the entire line by “id” or “location”, preserving valid
names such as video-api. At lines 322-335, 350-354, 385, and 448, replace broad
name substring matching with a key check that only accepts lines whose trimmed
key starts with name, preventing kms_key_name and filename from being selected.
- Around line 2363-2366: Update the duration_hours validation in the CLI
teardown update flow to reject zero as well as negative values by requiring a
strictly positive duration. Preserve the existing error message and typer.Exit
behavior for all invalid durations.
- Around line 1886-1890: Update the destroy flow around the cloud-specific
project_id assignment and subsequent gcloud configuration call so non-GCP
providers do not access an unbound project_id. Guard the project echo, gcloud
config set invocation, and related GCP-only operations behind the cloud == "gcp"
condition, or exit early with a clear unsupported-provider message before those
operations; preserve the existing GCP behavior.
- Around line 1107-1131: Update the GKE deployment branch in deploy to read the
configured gke.namespace and pass it as the namespace argument to both
deploy_to_gke calls for MLflow and FastAPI, matching the existing gke_apply
behavior and preserving the configured namespace consistently.
In `@src/deployml/terraform/modules/teardown/cloud/gcp/cloud_function/main.py`:
- Around line 49-58: Harden download_terraform_files so every blob path remains
within terraform_dir before any directory creation or download. Reject absolute
paths, paths containing traversal components, and resolved destinations that
escape via symlinks; validate the normalized target against the resolved
terraform_dir and abort invalid entries before filesystem writes.
In `@src/deployml/utils/helpers.py`:
- Around line 586-590: Initialize progress_percent before the process polling
loop in the surrounding Terraform apply helper, using the existing default
progress value expected by progress.update. Keep the loop’s later assignments
unchanged so the non-zero returncode branch can always report “Terraform apply
returned code” even when the process exits before the first poll.
In `@src/deployml/utils/kubernetes_local.py`:
- Around line 519-532: Update the minikube image-loading call in the function
containing the shown run_tool invocation to remove check=True, allowing non-zero
results to reach the existing return-code branch. Preserve the success message
and ensure failed loads return False so callers such as
generate_fastapi_manifests and generate_mlflow_manifests do not receive an
exception.
---
Nitpick comments:
In `@src/deployml/cli/cli.py`:
- Line 175: Remove the redundant local json imports from the functions around
the reported locations, including upload_resource_manifest, and rely on the
existing module-level import at line 16.
- Around line 206-219: Remove the no-op pass from the outputs loop and simplify
the storage bucket condition by eliminating the redundant isinstance/string
truthiness check after output_val has already been validated. Preserve the
existing bucket append behavior for keys containing "_bucket" with non-empty
output values.
- Around line 2369-2372: Move the timezone import to the existing module-level
datetime imports, then remove all local timezone and timezone-as-_tz imports in
the affected CLI code. Update those call sites to use timezone.utc directly,
including the teardown_at calculation near now and duration_hours, while
preserving existing behavior.
- Around line 486-532: The Terraform bucket lookup is duplicated between
upload_resource_manifest and upload_terraform_files_to_gcs. Extract the shared
state-list/state-show parsing into a _resolve_teardown_bucket(terraform_dir) ->
Optional[str] helper, including the existing error handling and bucket-name
extraction, then replace both functions’ inline logic with calls to that helper.
- Around line 2192-2243: Replace the duplicated Cloud Scheduler query and
parsing in show_teardown_status with deployml.api.get_teardown_status, using its
returned status data for CLI output while preserving the existing fallback
presentation. Apply the same delegation to update_teardown_schedule by calling
the corresponding API function and keeping that command focused on presentation;
remove direct gcloud invocation and scheduleTime/lastAttemptTime parsing from
both CLI functions.
In `@src/deployml/utils/teardown.py`:
- Around line 31-44: Remove the unused calculate_teardown_schedule function from
the teardown utilities module, including its associated documentation and
implementation; leave all remaining teardown functionality unchanged.
🪄 Autofix
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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 27747a48-1614-4461-bef2-d358ff7764ca
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (39)
.github/workflows/release.yml.github/workflows/test-pypi.yml.gitignore.pre-commit-config.yamldocs/contributing.mdexample/scripts/01_load_training_data.pyexample/scripts/02_train_model.pyexample/scripts/03_register_model.pyexample/scripts/04_make_predictions.pyexample/scripts/05_generate_ground_truth.pyexample/scripts/06_compute_drift_metrics.pyexample/scripts/07_setup_grafana.pypoetry.tomlpyproject.tomlsrc/deployml/__init__.pysrc/deployml/api.pysrc/deployml/cli/cli.pysrc/deployml/diagnostics/__init__.pysrc/deployml/diagnostics/doctor.pysrc/deployml/docker/fastapi/main.pysrc/deployml/notebook/__init__.pysrc/deployml/notebook/deployment.pysrc/deployml/notebook/display.pysrc/deployml/notebook/docker.pysrc/deployml/notebook/stack.pysrc/deployml/notebook/urls.pysrc/deployml/terraform/modules/teardown/cloud/gcp/cloud_function/main.pysrc/deployml/utils/constants.pysrc/deployml/utils/helpers.pysrc/deployml/utils/infracost.pysrc/deployml/utils/kubernetes_gke.pysrc/deployml/utils/kubernetes_local.pysrc/deployml/utils/platform_compat.pysrc/deployml/utils/teardown.pytests/test_doctor.pytests/test_gke_destroy.pytests/test_helpers.pytests/test_platform_compat.pytests/test_smoke.py
💤 Files with no reviewable changes (3)
- poetry.toml
- .github/workflows/test-pypi.yml
- .gitignore
| @@ -0,0 +1,103 @@ | |||
| name: Test and Release | |||
|
|
|||
| on: | |||
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- workflow ---'
cat -n .github/workflows/release.yml
printf '%s\n' '--- permission-related workflow entries ---'
rg -n -C 3 'permissions:|actions/checkout|uv sync|uv run|pull_request|push:' .github/workflows/release.ymlRepository: deployml-core/deployml
Length of output: 5901
Security Misconfiguration (CWE-269): Improper Privilege Management
Reachability: External
Set explicit baseline token permissions.
Add workflow-level permissions: { contents: read } for the lint and test jobs. Keep the existing release-job override for publishing and release commits.
🤖 Prompt for 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.
In @.github/workflows/release.yml at line 3, Add workflow-level permissions
granting only contents: read for the lint and test jobs in the release workflow.
Preserve the existing release-job permissions override so publishing and release
commits retain their required access.
Source: Linters/SAST tools
| lint: | ||
| runs-on: ubuntu-latest | ||
| steps: | ||
| - uses: actions/checkout@v4 |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
file=".github/workflows/release.yml"
printf '%s\n' '--- workflow context ---'
nl -ba "$file" | sed -n '1,95p'
printf '%s\n' '--- checkout and pull_request occurrences ---'
rg -n -C 3 'pull_request|actions/checkout|uv sync|uv run pytest|persist-credentials|GITHUB_TOKEN|semantic-release' "$file"Repository: deployml-core/deployml
Length of output: 2495
🏁 Script executed:
#!/bin/bash
set -eu
file=".github/workflows/release.yml"
printf '%s\n' '--- workflow ---'
sed -n '1,110p' "$file"
printf '%s\n' '--- repository references to this workflow ---'
rg -n -C 2 'release\.yml|pull_request|persist-credentials' .github README.md 2>/dev/null || true
printf '%s\n' '--- bounded workflow verifier ---'
python3 - <<'PY'
from pathlib import Path
p = Path(".github/workflows/release.yml")
lines = p.read_text().splitlines()
event = None
job = None
checkouts = []
commands = []
for i, raw in enumerate(lines, 1):
s = raw.strip()
if s == "pull_request:":
event = "pull_request"
if raw.startswith(" ") and not raw.startswith(" ") and s.endswith(":"):
job = s[:-1]
if s.startswith("- uses: actions/checkout@"):
with_block = []
for nxt in lines[i:i+5]:
if nxt.startswith(" ") or nxt.strip() == "":
with_block.append(nxt.strip())
else:
break
checkouts.append((i, job, event, with_block))
if s.startswith("run:"):
commands.append((i, job, event, s[4:].strip()))
print({"pull_request_event_seen": event == "pull_request"})
print({"checkouts": checkouts})
print({"commands": [x for x in commands if x[1] in {"lint", "test", "release"}]})
PYRepository: deployml-core/deployml
Length of output: 3946
Sensitive Data Exposure (CWE-522): Insufficiently Protected Credentials
Reachability: External · Exploitability: Trivial
Disable credential persistence in pull-request jobs.
Add persist-credentials: false to the actions/checkout@v4 steps at lines 20 and 41. Pull-request code runs uv sync and uv run pytest, so it can read the persisted GITHUB_TOKEN from Git configuration. Keep credential persistence in the release job because semantic-release pushes commits and tags.
🧰 Tools
🪛 zizmor (1.29.0)
[warning] 20-20: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
📍 Affects 1 file
.github/workflows/release.yml#L20-L20(this comment).github/workflows/release.yml#L41-L41
🤖 Prompt for 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.
In @.github/workflows/release.yml at line 20, Update the actions/checkout@v4
steps at .github/workflows/release.yml lines 20-20 and 41-41 to set
persist-credentials to false for both pull-request jobs. Leave the release job’s
checkout configuration unchanged so semantic-release can continue pushing
commits and tags.
Source: Linters/SAST tools
| - name: Sync dependencies | ||
| run: uv sync |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- workflow ---'
cat -n .github/workflows/release.yml | sed -n '1,105p'
printf '%s\n' '--- uv configuration and lock usage ---'
rg -n --hidden -g '!**/.git/**' 'uv (sync|lock)|locked|frozen|uv\.lock|PACKAGE_NAME' .Repository: deployml-core/deployml
Length of output: 5201
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- pyproject release configuration ---'
cat -n pyproject.toml | sed -n '1,95p'
printf '%s\n' '--- lockfile and project metadata ---'
ls -l uv.lock pyproject.toml
sed -n '1,45p' uv.lockRepository: deployml-core/deployml
Length of output: 5825
🌐 Web query:
uv sync --locked documentation behavior stale lockfile
💡 Result:
When you use the --locked flag with uv sync, it instructs uv to assert that the existing uv.lock file is up-to-date with your current project configuration (such as pyproject.toml) [1][2]. If the lockfile is missing or is not up-to-date—meaning your project's dependency constraints have changed in a way that requires the lockfile to be regenerated—uv will exit with an error instead of attempting to update the lockfile [1][2][3]. This behavior is designed to ensure that the environment is strictly reproducible and to prevent unintended modifications to the lockfile in environments where that is restricted, such as CI/CD pipelines or production deployments [1][2]. In summary: * Behavior: uv verifies the lockfile against current project metadata [1][4]. * Stale Lockfile: If the lockfile is stale (out of sync with dependencies), the command fails with an error [1][3]. * Use Case: This is best for environments where you want to ensure the installation matches the existing lockfile exactly and fail if it does not [2][4]. If you intend to bypass the requirement that the lockfile must match the project's dependency constraints and simply want to install based on the existing lockfile without modification, you should use --frozen instead [2][4][3]. While --locked validates the lockfile's integrity, --frozen treats the lockfile as the immutable source of truth and ignores any potential drifts in the project's dependency definitions [2][3].
Citations:
- 1: https://docs.astral.sh/uv/concepts/projects/sync/
- 2: https://docs.rs/uv-cli/latest/uv_cli/struct.SyncArgs.html
- 3: uv sync should warn (or fail) when lock file is out of sync with pyproject.toml dependency tables astral-sh/uv#11530
- 4: Frozen vs Locked unexpected behavior astral-sh/uv#9379
🏁 Script executed:
#!/bin/bash
set -eu
python3 - <<'PY'
from pathlib import Path
workflow = Path(".github/workflows/release.yml").read_text()
pyproject = Path("pyproject.toml").read_text()
sync_runs = [line.strip() for line in workflow.splitlines() if line.strip() == "run: uv sync"]
print("plain uv sync steps:", len(sync_runs))
print("all expected sync sites present:", len(sync_runs) == 3)
print('semantic-release retains uv lock upgrade:', 'uv lock --upgrade-package "$PACKAGE_NAME"' in pyproject)
PY
if command -v uv >/dev/null 2>&1; then
uv --version
uv sync --help | grep -E -- '--locked|--frozen' || true
else
echo "uv is not available in the verifier sandbox"
fiRepository: deployml-core/deployml
Length of output: 312
Make CI reject a stale uv.lock.
Use uv sync --locked at all three dependency-sync steps. Keep uv lock --upgrade-package "$PACKAGE_NAME" in the semantic-release build command because that path intentionally updates uv.lock.
📍 Affects 1 file
.github/workflows/release.yml#L28-L29(this comment).github/workflows/release.yml#L49-L50.github/workflows/release.yml#L76-L77
🤖 Prompt for 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.
In @.github/workflows/release.yml around lines 28 - 29, Update all three
dependency-sync steps in .github/workflows/release.yml at lines 28-29, 49-50,
and 76-77 to run the locked sync mode so CI rejects a stale uv.lock. Leave the
semantic-release build command’s uv lock --upgrade-package "$PACKAGE_NAME"
invocation unchanged because it intentionally updates the lockfile.
| - name: Run tests | ||
| run: uv run pytest |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- workflow ---'
cat -n .github/workflows/release.yml | sed -n '1,120p'
printf '%s\n' '--- project files ---'
git ls-files | sed -n '1,160p'
printf '%s\n' '--- packaging and test configuration ---'
for f in pyproject.toml setup.cfg setup.py tox.ini noxfile.py; do
if [ -f "$f" ]; then
printf '%s\n' "--- $f ---"
cat -n "$f" | sed -n '1,220p'
fi
done
printf '%s\n' '--- smoke test ---'
if [ -f tests/test_smoke.py ]; then
cat -n tests/test_smoke.py | sed -n '1,220p'
fiRepository: deployml-core/deployml
Length of output: 14974
🏁 Script executed:
#!/bin/bash
set -eu
python3 - <<'PY'
from pathlib import Path
import re
workflow = Path(".github/workflows/release.yml").read_text()
pyproject = Path("pyproject.toml").read_text()
tests = sorted(str(p) for p in Path("tests").glob("test_*.py"))
print("smoke_test_present:", Path("tests/test_smoke.py").is_file())
print("test_files:", tests)
print("test_job_syncs:", bool(re.search(r"(?ms)^ test:.*?^\s+run: uv sync$", workflow)))
print("test_job_runs_pytest:", bool(re.search(r"(?ms)^ test:.*?^\s+run: uv run pytest$", workflow)))
print("release_builds_wheel:", "uv build" in pyproject)
print("release_build_command:")
match = re.search(r'(?ms)^\s*build_command\s*=\s*"""\n(.*?)\n\s*"""', pyproject)
print(match.group(1) if match else "<not found>")
PY
printf '%s\n' '--- repository status ---'
git status --short
printf '%s\n' '--- test references ---'
rg -n 'test_smoke|uv sync|uv build|semantic-release|editable' .github pyproject.toml tests README.md docs || trueRepository: deployml-core/deployml
Length of output: 1631
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- smoke test and related tests ---'
cat -n tests/test_smoke.py | sed -n '1,140p'
printf '%s\n' '--- lockfile project entry ---'
rg -n -A18 -B4 'name = "deployml-core"|source = \{ editable' uv.lock || true
printf '%s\n' '--- relevant packaging docs ---'
cat -n docs/contributing.md | sed -n '1,60p'
printf '%s\n' '--- uv availability ---'
if command -v uv >/dev/null 2>&1; then
uv --version
else
printf '%s\n' 'uv unavailable in sandbox'
fiRepository: deployml-core/deployml
Length of output: 5794
Test the built wheel before release.
uv sync installs the project in editable mode, so the smoke tests do not validate the wheel. Build and install the wheel in a clean environment, then run tests/test_smoke.py before publishing.
🧰 Tools
🪛 zizmor (1.29.0)
[warning] 38-53: overly broad permissions (excessive-permissions): default permissions used due to no permissions: block
(excessive-permissions)
🤖 Prompt for 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.
In @.github/workflows/release.yml around lines 52 - 53, Update the release
workflow’s “Run tests” step to build the wheel, install that wheel in a clean
environment, and execute tests/test_smoke.py against the installed artifact
before publishing. Do not rely on the editable installation created by uv sync.
| if ( | ||
| ( | ||
| clean_line.strip().startswith("name") | ||
| or " = name" in clean_line.lower() | ||
| ) | ||
| and "=" in clean_line | ||
| and "location" not in clean_line.lower() | ||
| and "id" not in clean_line.lower() | ||
| and "latest" not in clean_line.lower() | ||
| ): | ||
| parts = clean_line.split("=") | ||
| if len(parts) >= 2: | ||
| potential_name = parts[1].strip().strip('"').strip("'") | ||
| potential_name = ( | ||
| parts[1].strip().strip('"').strip("'") | ||
| ) | ||
| # Remove ANSI codes from the name itself | ||
| potential_name = re.sub(r'\x1b\[[0-9;]*m', '', potential_name) | ||
| potential_name = re.sub( | ||
| r"\x1b\[[0-9;]*m", "", potential_name | ||
| ) | ||
| # Skip null, empty, or invalid values | ||
| if potential_name and potential_name.lower() != 'null' and '/' not in potential_name and '@' not in potential_name and len(potential_name) < 200: | ||
| if ( | ||
| potential_name | ||
| and potential_name.lower() != "null" | ||
| and "/" not in potential_name | ||
| and "@" not in potential_name | ||
| and len(potential_name) < 200 | ||
| ): | ||
| service_name = potential_name | ||
| break |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
extract_resource_manifest parses terraform state show text with unreliable substring matching. Both sites split raw output on = and test substrings against the whole line, so the recorded resource name can be wrong or missing. The uploaded manifest then drives scheduled teardown, and a wrong or missing name means the resource is never deleted and keeps billing. The single fix is to match the attribute key exactly, or to read terraform show -json and use values.name per resource.
src/deployml/cli/cli.py#L265-L293: replace the whole-line"id" not in clean_line.lower()and"location" not in clean_line.lower()tests with an exact key comparison, so a service name that containsid, for examplevideo-api, is still recorded.src/deployml/cli/cli.py#L322-L335: replace"name" in line.lower()withline.strip().startswith("name")in the Cloud Run Job branch, and apply the same change in the scheduler branch at Lines 350-354, the Pub/Sub branch at Line 385, and the Cloud Build trigger branch at Line 448, wherekms_key_nameandfilenamecurrently win the match.
📍 Affects 1 file
src/deployml/cli/cli.py#L265-L293(this comment)src/deployml/cli/cli.py#L322-L335
🤖 Prompt for 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.
In `@src/deployml/cli/cli.py` around lines 265 - 293, Update
extract_resource_manifest in src/deployml/cli/cli.py at lines 265-293 to parse
the attribute key exactly instead of filtering the entire line by “id” or
“location”, preserving valid names such as video-api. At lines 322-335, 350-354,
385, and 448, replace broad name substring matching with a key check that only
accepts lines whose trimmed key starts with name, preventing kms_key_name and
filename from being selected.
| run_tool( | ||
| "gcloud", ["config", "set", "project", project_id], | ||
| "gcloud", | ||
| ["config", "set", "project", project_id], | ||
| cwd=DEPLOYML_TERRAFORM_DIR, | ||
| ) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
project_id can be unbound for a non-GCP provider.
destroy assigns project_id only inside the if cloud == "gcp": branch at Line 1869. Every later use, including this gcloud config set project call and the echo at Line 1875, runs unconditionally. A config with provider.name set to aws or azure passes _validate_deploy_config_or_exit, because that function requires project_id for gcp only. destroy then raises NameError before the try block at Line 1882, so the user sees a traceback instead of a message.
Guard the cloud-specific path, or exit early with a clear message when cloud != "gcp".
🐛 Proposed fix
cloud = config["provider"]["name"]
- if cloud == "gcp":
- project_id = config["provider"]["project_id"]
+ if cloud != "gcp":
+ typer.secho(
+ f" destroy currently supports the gcp provider only, got '{cloud}'.",
+ fg=typer.colors.RED,
+ )
+ raise typer.Exit(code=1)
+ project_id = config["provider"]["project_id"]🤖 Prompt for 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.
In `@src/deployml/cli/cli.py` around lines 1886 - 1890, Update the destroy flow
around the cloud-specific project_id assignment and subsequent gcloud
configuration call so non-GCP providers do not access an unbound project_id.
Guard the project echo, gcloud config set invocation, and related GCP-only
operations behind the cloud == "gcp" condition, or exit early with a clear
unsupported-provider message before those operations; preserve the existing GCP
behavior.
| # Duration is now passed in via CLI flag instead of interactive prompt. | ||
| if duration_hours < 0: | ||
| typer.echo(" Duration must be positive") | ||
| raise typer.Exit(code=1) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Reject duration_hours of 0 as the message states.
The check allows 0, but the message says the duration must be positive. deployml teardown update --hours 0 then sets a cron schedule for the current minute and tears the stack down immediately.
🐛 Proposed fix
# Duration is now passed in via CLI flag instead of interactive prompt.
- if duration_hours < 0:
+ if duration_hours <= 0:
typer.echo(" Duration must be positive")
raise typer.Exit(code=1)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Duration is now passed in via CLI flag instead of interactive prompt. | |
| if duration_hours < 0: | |
| typer.echo(" Duration must be positive") | |
| raise typer.Exit(code=1) | |
| # Duration is now passed in via CLI flag instead of interactive prompt. | |
| if duration_hours <= 0: | |
| typer.echo(" Duration must be positive") | |
| raise typer.Exit(code=1) |
🤖 Prompt for 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.
In `@src/deployml/cli/cli.py` around lines 2363 - 2366, Update the duration_hours
validation in the CLI teardown update flow to reject zero as well as negative
values by requiring a strictly positive duration. Preserve the existing error
message and typer.Exit behavior for all invalid durations.
| # Download terraform files from GCS if provided | ||
| if terraform_files_bucket: | ||
| download_terraform_files(terraform_files_bucket, workspace_name, terraform_dir) | ||
|
|
||
| download_terraform_files( | ||
| terraform_files_bucket, workspace_name, terraform_dir | ||
| ) | ||
|
|
||
| # Download terraform state if stored in GCS | ||
| if terraform_state_bucket: | ||
| download_terraform_state(terraform_state_bucket, workspace_name, terraform_dir) | ||
|
|
||
| download_terraform_state( | ||
| terraform_state_bucket, workspace_name, terraform_dir |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- candidate files ---'
fd -i -t f 'main.py|helpers.py' src/deployml/terraform/modules/teardown src/deployml 2>/dev/null | head -80
printf '%s\n' '--- download function definitions and callers ---'
rg -n -A80 -B12 'def download_terraform_(files|state)|download_terraform_(files|state)\(' src
printf '%s\n' '--- relevant file outline ---'
ast-grep outline src/deployml/terraform/modules/teardown/cloud/gcp/cloud_function/main.py 2>/dev/null || trueRepository: deployml-core/deployml
Length of output: 19502
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- request entry point and bucket fields ---'
sed -n '1,48p' src/deployml/terraform/modules/teardown/cloud/gcp/cloud_function/main.py
rg -n -A12 -B8 'terraform_files_bucket|terraform_state_bucket|teardown_infrastructure|cloud_function' src tests 2>/dev/null | head -240
printf '%s\n' '--- path-behavior verifier ---'
python3 - <<'PY'
from pathlib import Path
import tempfile
with tempfile.TemporaryDirectory() as t:
root = Path(t) / "terraform"
root.mkdir()
cases = [
"main.tf",
"../../target",
"/tmp/target",
"nested/../../target",
]
for relative_path in cases:
target = root / relative_path
print(f"{relative_path!r} -> {target} | resolved={target.resolve()} | inside={root.resolve() in target.resolve().parents or target.resolve() == root.resolve()}")
PYRepository: deployml-core/deployml
Length of output: 24795
Path Traversal (CWE-22): Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal')
Exploitability: Moderate
Keep downloaded object paths inside terraform_dir.
download_terraform_files joins each unvalidated blob path with terraform_dir, allowing .. components to write outside it. Reject absolute paths, traversal paths, and symlink escapes before creating directories or downloading files.
🤖 Prompt for 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.
In `@src/deployml/terraform/modules/teardown/cloud/gcp/cloud_function/main.py`
around lines 49 - 58, Harden download_terraform_files so every blob path remains
within terraform_dir before any directory creation or download. Reject absolute
paths, paths containing traversal components, and resolved destinations that
escape via symlinks; validate the normalized target against the resolved
terraform_dir and abort invalid entries before filesystem writes.
Source: Linters/SAST tools
| progress.update( | ||
| task, | ||
| completed=progress_percent, | ||
| description=f"⚠️ Terraform apply returned code {returncode}", | ||
| ) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
progress_percent can be unbound in the failure branch.
progress_percent is assigned only inside the while process.poll() is None: loop at Lines 564-570. A terraform process that exits before the first poll never enters the loop. The non-zero return code then reaches this progress.update call and raises NameError. The finally block closes the log file, but the exception propagates to deploy, which reports a traceback instead of the intended "Terraform apply returned code N" message.
A fast failure, for example an immediate provider or validation error, is exactly the case that reaches this branch.
Initialize progress_percent before the loop.
🐛 Proposed fix
start_time = time.time()
estimated_seconds = estimated_minutes * 60
n_msgs = len(resource_msgs)
+ progress_percent = 0
while process.poll() is None:🤖 Prompt for 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.
In `@src/deployml/utils/helpers.py` around lines 586 - 590, Initialize
progress_percent before the process polling loop in the surrounding Terraform
apply helper, using the existing default progress value expected by
progress.update. Keep the loop’s later assignments unchanged so the non-zero
returncode branch can always report “Terraform apply returned code” even when
the process exits before the first poll.
| result = run_tool( | ||
| "minikube", ["image", "load", image_name], | ||
| "minikube", | ||
| ["image", "load", image_name], | ||
| check=True, | ||
| capture_output=True, | ||
| text=True | ||
| text=True, | ||
| ) | ||
|
|
||
| if result.returncode == 0: | ||
| typer.echo(f"✅ Image '{image_name}' loaded into minikube") | ||
| return True | ||
| else: | ||
| typer.echo(f"❌ Failed to load image: {result.stderr}") | ||
| return False No newline at end of file | ||
| return False |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
check=True makes the failure branch unreachable and raises instead of returning False.
run_tool forwards check=True to subprocess.run, so a non-zero exit raises CalledProcessError before Line 527 executes. The else branch at Line 530 is dead code, and the documented False return never happens on a failed load.
generate_fastapi_manifests (Line 81) and generate_mlflow_manifests (Line 154) call this function without a handler, so a failed minikube image load aborts minikube-init with a traceback.
Drop check=True and keep the return-code branch.
🐛 Proposed fix
result = run_tool(
"minikube",
["image", "load", image_name],
- check=True,
capture_output=True,
text=True,
)
if result.returncode == 0:
typer.echo(f"✅ Image '{image_name}' loaded into minikube")
return True
else:
typer.echo(f"❌ Failed to load image: {result.stderr}")
return False📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| result = run_tool( | |
| "minikube", ["image", "load", image_name], | |
| "minikube", | |
| ["image", "load", image_name], | |
| check=True, | |
| capture_output=True, | |
| text=True | |
| text=True, | |
| ) | |
| if result.returncode == 0: | |
| typer.echo(f"✅ Image '{image_name}' loaded into minikube") | |
| return True | |
| else: | |
| typer.echo(f"❌ Failed to load image: {result.stderr}") | |
| return False | |
| \ No newline at end of file | |
| return False | |
| result = run_tool( | |
| "minikube", | |
| ["image", "load", image_name], | |
| capture_output=True, | |
| text=True, | |
| ) | |
| if result.returncode == 0: | |
| typer.echo(f"✅ Image '{image_name}' loaded into minikube") | |
| return True | |
| else: | |
| typer.echo(f"❌ Failed to load image: {result.stderr}") | |
| return False |
🤖 Prompt for 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.
In `@src/deployml/utils/kubernetes_local.py` around lines 519 - 532, Update the
minikube image-loading call in the function containing the shown run_tool
invocation to remove check=True, allowing non-zero results to reach the existing
return-code branch. Preserve the success message and ensure failed loads return
False so callers such as generate_fastapi_manifests and
generate_mlflow_manifests do not receive an exception.
2 participants
Summary
Replaces Poetry with the uv build backend, and replaces manual run-number versioning with python-semantic-release (PSR) driven by Conventional Commits.
pyproject.toml: build backend switched touv_build(waspoetry-core), Poetry-specific packaging config removed,[tool.semantic_release]config added,devdependency group added (pytest,python-semantic-release)..github/workflows/release.ymlreplacestest-pypi.yml: runs tests on every push/PR; on push tomain, PSR bumps the version, updates the changelog, tags the release, publishes to PyPI via OIDC trusted publishing (no stored token), and attaches build artifacts to the GitHub Release.poetry.toml; stopped ignoringpoetry.lock, now trackinguv.lock.tests/test_smoke.pyas the CI test gate.docs/contributing.md.Verified locally
uv buildproduces a 182KB wheel (previously 41MB, due to a stray local.terraformprovider binary now excluded);docker/,templates/,terraform/package assets are all present.uv run pytestpasses.uv lock --checkis in sync.Known gap
mainhas no unit test suite, so only the smoke test runs in CI for now. The real suite lives ondev, which has already been merged intomain(PR #66) but the tests themselves weren't part of that merge and still need to be ported/adapted separately.Before this can auto-release (one-time setup, not code)
deployml-coreon pypi.org (repodeployml-core/deployml, workflowrelease.yml, jobrelease). Required for OIDCuv publishto work — without it, the first release push will fail at the publish step.pyproject.toml: currently0.0.60, but the latest published/tagged version is0.0.67. Confirm this doesn't conflict with how PSR resolves the current version (it may just take the max across tags and the toml value, but worth checking before the first release run rather than after).PYPI_API_TOKENandTEST_PYPI_API_TOKENare no longer used (TestPyPI publishing was dropped) and can be removed.maincurrently has no branch protection rules, so PSR's release commit/tag push needs no bypass. If protection is added later, make sure the bot/token can still push tomain.Test plan
testjob runs pytest on this PRreleasejob bumps the version, publishes to PyPI, tags the release, and creates the GitHub Release with changelogSummary by CodeRabbit
New Features
Documentation
Chores