Skip to content

Add UpliftAI key to CI tests#65

Merged
willwade merged 1 commit intomainfrom
codex/fix-github-actions-upliftai_key-issue
Aug 24, 2025
Merged

Add UpliftAI key to CI tests#65
willwade merged 1 commit intomainfrom
codex/fix-github-actions-upliftai_key-issue

Conversation

@willwade
Copy link
Owner

Summary

  • expose UPLIFTAI_KEY to the test workflow
  • document development workflow in AGENTS.md

Testing

  • uv sync --all-extras
  • uv run postinstall
  • POLLY_REGION=dummy POLLY_AWS_KEY_ID=dummy POLLY_AWS_ACCESS_KEY=dummy GOOGLE_SA_PATH=google_creds.json GOOGLE_SA_FILE_B64=e30= MICROSOFT_TOKEN=dummy MICROSOFT_REGION=dummy WATSON_API_KEY=dummy WATSON_REGION=dummy WATSON_INSTANCE_ID=dummy ELEVENLABS_API_KEY=dummy WITAI_TOKEN=dummy PLAYHT_API_KEY=dummy PLAYHT_USER_ID=dummy UPLIFTAI_KEY=dummy uv run pytest -m "not synthetic and not sapi and not watson"

https://chatgpt.com/codex/tasks/task_e_68ab73b9fa748327919964aea380d217

@willwade willwade merged commit 23cb728 into main Aug 24, 2025
1 check passed
Copy link

@llamapreview llamapreview bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Auto Pull Request Review from LlamaPReview

🎯 TL;DR & Recommendation

Recommendation: Approve with suggestions
This PR successfully adds UpliftAI key to CI tests and documents the workflow, but includes minor suggestions to enhance documentation clarity and workflow robustness.

🌟 Strengths

  • Follows existing patterns for secret management in GitHub Actions.
  • Comprehensive documentation of the development workflow.

💡 Suggestions (P2)

  • AGENTS.md: Lacks specific guidance on handling missing credentials, which could confuse contributors when troubleshooting test failures.
  • .github/workflows/pythonpackage.yml: Missing validation for secrets could lead to cryptic error messages, slowing down issue resolution.
  • AGENTS.md: Incomplete instruction on updating test filters might result in new engines being accidentally excluded from CI runs, reducing test coverage.

💡 LlamaPReview Community
Have feedback on this AI Code review tool? Join our GitHub Discussions to share your thoughts and help shape the future of LlamaPReview.

uv run pytest -m "not synthetic and not sapi and not watson"
```

- The test suite requires credentials for several TTS engines. Provide
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 | Confidence: High

This instruction is clear but incomplete. It doesn't specify whether contributors should also update the test command's filter flags (e.g., -m "not synthetic and not sapi and not watson") when adding new engines. This could lead to inconsistent test coverage if new engines are accidentally excluded from CI runs due to outdated filter flags.

Code Suggestion:

Expand the documentation to include guidance on updating test filters when adding new engines, and consider linking to the pytest marker definitions for clarity.

@@ -0,0 +1,21 @@
# Instructions for Contributors
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 | Confidence: Medium

The documentation correctly captures the development workflow but lacks specific guidance on handling missing credentials. The test command uses -m "not synthetic and not sapi and not watson" to exclude certain engines, but contributors might not understand why these are excluded or how to handle other engines with missing credentials. This could lead to confusion when adding new engines or troubleshooting test failures. The note about GitHub Actions secrets is good but should be complemented with local development guidance.

@@ -95,6 +95,7 @@ jobs:
GOOGLE_SA_FILE_B64: ${{ secrets.GOOGLE_SA_FILE_B64 }}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Contextual Comment]
This comment refers to code near real line 83. Anchored to nearest_changed(95) line 95.


P2 | Confidence: High

The addition of UPLIFTAI_KEY follows the existing pattern for secret management, which is good. However, the workflow does not include any validation to ensure required secrets are present before attempting tests. If a secret is missing (e.g., not configured in GitHub), the tests will fail with cryptic errors rather than clear feedback about missing environment variables. This could slow down troubleshooting for maintainers.

Code Suggestion:

Consider adding a pre-test step to validate that all required secrets are non-empty, or implement graceful skipping of tests for missing credentials with clear log messages.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant