fix(publishing): guard reserved bundle paths - #5573
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
🟡 Changes recommended
Reserved-path checks still miss parent-path and bundled-library collisions that can corrupt published bundles.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Adds collision protection for reserved workflow bundle paths and returns structured packaging metadata.
Changes:
- Validates entrypoint and static-file destinations against reserved paths.
- Adds extensible publisher-specific reservations.
- Introduces
PackagedBundleand documents the breaking return-type change.
File summaries
| File | Description |
|---|---|
workflow_packager.py |
Adds collision validation and structured results. |
publishing/__init__.py |
Exports the new public API. |
test_workflow_packager.py |
Adds unit coverage for reservations and results. |
test_workflow_bundle.py |
Updates end-to-end tests for PackagedBundle. |
MIGRATION.md |
Documents the return-type migration. |
docs/guides/publishing.md |
Documents publisher integration. |
Review details
- Files reviewed: 6/6 changed files
- Comments generated: 2
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
507f59d to
068c248
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Parent-directory segments can bypass the new reserved-path collision guard.
Get a fresh assessment by requesting another Copilot review.
Review details
- Files reviewed: 6/6 changed files
- Comments generated: 1
- Review effort level: Balanced
Part of #5323 / #5326. Fail loudly if the bundled workflow or a static file referenced by the workflow has a file name that clashes with a file reserved by the bundle. Allow the list of reserved files to be extended by external publishers by adding an optional `additional_reserved_paths` when calling the `package_to_folder()` helper. Return a more useful value from `package_to_folder()`, containing the final workflow path. This is technically a breaking change, but no known publisher makes use of this value. External publishers can then use this to stop reconstructing the entrypoint workflow path. It will be especially useful if/when no longer flattening the workflow #5324, and when disambiguating subflows alongside the entrypoint workflow in #5180.
068c248 to
bb01b31
Compare
There was a problem hiding this comment.
🟡 Changes recommended
The new result contract can expose absolute publishing-machine library paths for sandbox or non-JSON libraries.
Get a fresh assessment by requesting another Copilot review.
Review details
- Files reviewed: 6/6 changed files
- Comments generated: 1
- Review effort level: Balanced
| return library_paths | ||
| return PackagedBundle( | ||
| entrypoint_workflow_path=entrypoint_workflow_path, | ||
| library_paths=tuple(Path(library_path) for library_path in library_paths), |
There was a problem hiding this comment.
This is a separate issue with library copying behaviour. Note that sandbox libraries do have JSON paths. But nevertheless it is possible for a library to be registered as by directory rather than by .json file.
In the case of a library registered by directory, we could/should still copy the directory into the bundle, rather than skip it. But that's a different issue orthogonal to this PR.
In the meantime, we report the library list exactly as it was before this PR.
I think PackagedBundle documenting that paths are relative to the bundle is still reasonable, since it is understood that only applies if the path itself is relative.
Part of #5323 / #5326.
Fail loudly if the bundled workflow or a static file referenced by the workflow has a file name that clashes with a file reserved by the bundle.
Allow the list of reserved files to be extended by external publishers by adding an optional
additional_reserved_pathswhen calling thepackage_to_folder()helper.Return a more useful value from
package_to_folder(), containing the final workflow path. This is technically a breaking change, but no known publisher makes use of this value. External publishers can then use this to stop reconstructing the entrypoint workflow path. It will be especially useful if/when no longer flattening the workflow #5324, and when disambiguating subflows alongside the entrypoint workflow in #5180.📚 Documentation preview 📚: https://griptape-nodes--5573.org.readthedocs.build/en/5573/