Conversation
|
Thank you for raising your pull request and contributing to voscript. |
|
Claude finished @MapleEve's task in 4m 53s —— View job PR Review: Add transcription artifact manifestTodo List
Review SummaryNo actionable issues found. This PR adds an optional What Was Reviewed✅ Privacy and Security
✅ HTTP API Compatibility
✅ Test Coverage
✅ Documentation Synchronization
✅ Model Lifecycle
Architecture Notes
Recommendation: Approve and merge. This PR follows VoScript's security and compatibility guidelines without introducing actionable risks. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #33 +/- ##
==========================================
+ Coverage 91.18% 91.24% +0.06%
==========================================
Files 76 76
Lines 3016 3039 +23
==========================================
+ Hits 2750 2773 +23
Misses 266 266
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Pull request overview
Adds a public-safe, optional artifacts manifest to completed transcription results so clients can discover result-adjacent files (e.g., result.json, speaker embeddings) without exposing host-local paths, and documents compatibility expectations for missing/unknown fields.
Changes:
- Introduce
ArtifactManifestEntry+build_artifact_manifest()(versioned asartifact_manifest.v1) and embed the manifest in completed transcription payloads. - Persist and validate the manifest via unit tests (including “no local path leakage” assertions).
- Expand zh/en API + configuration docs and update changelogs to describe the new optional field and compatibility rules.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tests/unit/test_pipeline_runner.py | Adds assertions for the artifacts manifest in API results and persisted result.json, including path-safety checks. |
| tests/conftest.py | Extends lightweight stubs (notably numpy + fastapi) to support more import/runtime scenarios in minimal environments. |
| doc/configuration.zh.md | Documents artifacts as an optional, public-safe completed-result field and clarifies compatibility expectations. |
| doc/configuration.en.md | Same as above for English documentation. |
| doc/changelog.zh.md | Notes the new optional artifacts manifest feature in the Unreleased section. |
| doc/changelog.en.md | Same as above for English changelog. |
| doc/api.zh.md | Adds artifacts to example payloads and documents the manifest semantics/compatibility. |
| doc/api.en.md | Same as above for the English API docs. |
| app/providers/artifacts/default.py | Builds and attaches the artifacts manifest (stable entries: result.json + per-speaker embedding files). |
| app/pipeline/contracts/artifacts.py | Defines the manifest contract (ARTIFACT_MANIFEST_VERSION, ArtifactManifestEntry, build_artifact_manifest). |
| app/pipeline/contracts/init.py | Re-exports new artifacts-manifest contract symbols. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| def __getitem__(self, item): | ||
| if isinstance(item, tuple): | ||
| value = self | ||
| for index in item: |
|
❤️ Great PR @MapleEve ❤️ The growth of project is inseparable from user feedback and contribution, thanks for your contribution! |
Summary
artifactsmanifest to completed transcription results.artifactsfields in zh/en API and configuration docs.Validation
ruff check app/ tests/conftest.py --ignore E501ruff format --check app/ tests/conftest.pyPYTEST_DISABLE_PLUGIN_AUTOLOAD=1 python -m pytest tests/unit/ tests/test_security.py -v --tb=short --no-header(150 passed)