Fixes on update-thv-models

[aponcedeleonch]

The script is failing currently. I will invesitgate by runnin in this branch/PR

[claude]

Review Feedback

✅ Positive Changes

• Better error handling: Process exit detection (L38-43) prevents silent failures
• Improved logging: More granular feedback during health check attempts
• Correct Python usage: Using uv run python instead of python3 (L50) aligns with project standards

🐛 Issues

1. Workflow dependency removal breaks functionality (.github/workflows/update-thv-models.yml:40-42)

• Removed task install step, but the script depends on Python packages (datamodel-codegen at L90)
• The generate-thv-models task has deps: [install] in Taskfile.yml, which won't execute in CI without explicit step
• Fix: Restore the install step or add explicit uv sync before running the task

2. Conditional logic flaw (L65-69)

• The if [ $i -eq $MAX_ATTEMPTS ] check occurs inside the loop, making it unreachable after successful connection
• Should be placed after the loop completes
• Current flow: Works but unnecessarily checks condition on every iteration

🔧 Suggested Improvements

scripts/generate_toolhive_models.sh:65-70 - Move max attempts check outside loop:

    done
    
    # Check moved after loop
    if [ $i -gt $MAX_ATTEMPTS ]; then
        echo "ERROR: thv serve did not become ready after $MAX_ATTEMPTS attempts"
        echo "Last response: $response"
        exit 1
    fi
fi

Remove temporary debugging trigger (L8-10) - Remember to remove the push trigger on fix-gha-generate-thv-models branch before merging

📊 Summary

• Critical: Restore dependency installation in workflow
• Minor: Refactor max attempts logic (works but suboptimal)
• Cleanup: Remove debug trigger before merge

---

Review completed in ~90 seconds

[aponcedeleonch]

Correctly works. PR generated: #23