fix(starlark): toggle the app id the list published, and store a relocatable star_file - #537
Merged
Merged
Conversation
…catable star_file The two review nitpicks left over from #535. Both are still on main after that merge; the five findings alongside them landed with it. **The toggle could not find what the list had just shown.** `_starlark_virtual_plugins` publishes the raw manifest key as `starlark:<key>`, and `_toggle_starlark_app` passed it back through `_validate_and_sanitize_app_id`, which lowercases and rewrites every character outside `[a-z0-9_]`. An app stored as `My-App` was listed as `starlark:My-App` and looked up as `my_app`, so toggling an app the page had drawn a moment earlier answered 404. Keys written by `_install_star_file` are already sanitised, so this only shows up for manifests written by the starlark-apps plugin itself or edited by hand. `_validate_starlark_app_path` rejects traversal without rewriting, so it is the check to use here -- listing and toggling now agree on one key. The updater also uses `setdefault` rather than indexing: the app is loaded but its on-disk entry need not exist, and `_update_manifest_safe` does not catch `KeyError`, so that escaped as a 500 rather than writing the entry. **`star_file` was stored absolute.** Readers join it to the app's own directory -- `_standalone_render_starlark_app` does `app_dir / app_data.get('star_file', f'{app_id}.star')` -- so the key's default is a bare filename and an absolute value gave it a second meaning. Since `Path.__truediv__` discards the left side when the right is absolute, the manifest was pinned to whatever PROJECT_ROOT installed it, and a moved or redeployed install could not find its own file. Storing `dest.name` matches the default and stays relocatable. Read paths are unchanged, so manifests already holding an absolute path keep working. 7 new tests. Whole suite: no new failures against main, 4013 passed against 4007. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Contributor
Warning Review limit reachedNext included review available in 21 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (2)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Up to standards ✅🟢 Issues |
Uh oh!
There was an error while loading. Please reload this page.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The two review nitpicks from #535 that the merge did not carry. The five findings raised alongside them landed with it; these are still on
main.The toggle could not find what the list had just shown
_starlark_virtual_pluginspublishes the raw manifest key asstarlark:<key>._toggle_starlark_apppassed it back through_validate_and_sanitize_app_id, which lowercases and replaces every character outside[a-z0-9_]:My-Appstarlark:My-Appmy_appKeys written by
_install_star_fileare already sanitised, so this only surfaces for manifests written by the starlark-apps plugin itself, or edited by hand._validate_starlark_app_pathrejects traversal without rewriting, so it is the right check here — listing and toggling now agree on one key. The traversal test keeps its assertion, retargeted at the wording that check actually returns.The updater also switches from indexing to
setdefault. The app is loaded but its on-disk entry need not exist, and_update_manifest_safedoes not catchKeyError— so that case escaped as a 500 instead of writing the entry.star_filewas stored as an absolute pathIts readers join it to the app's own directory, and default to a bare filename:
So the key had two meanings depending on who wrote it.
Path.__truediv__discards the left side when the right is absolute, which pinned the manifest to whateverPROJECT_ROOTinstalled it — a moved or redeployed install could not find its own file. Storingdest.namematches the default and stays relocatable.Read paths are untouched, so manifests already holding an absolute path keep working.
Testing
7 new tests: the published id round-trips through the toggle, a loaded app missing from the manifest no longer 500s, and
star_fileis stored by name, is not absolute, and resolves against the app directory.Whole suite on this machine: no new failures against
main(104 pre-existing Windows-only in both), 4013 passed against 4007.🤖 Generated with Claude Code