Gate dataset-level Galaxy delete/purge (follow-up to #338)#345
Merged
Conversation
…t#338) galaxyproject#338 added the confirmation gate for whole-history delete/purge but deliberately scoped dataset-level deletes out -- the curl guardrail skipped the /contents/ sub-path so it wouldn't mislabel a dataset delete as "entire history". A dataset purge is still irreversible data loss, just smaller, so this extends the same classifier to catch a curl/wget DELETE against /api/histories/{id}/contents/{dsid}: a plain delete reads as a recoverable soft-delete, a purge (query ?purge=true or request body) as irreversible, each with dataset-scoped wording instead of the whole-history text. No gate/policy changes were needed -- both gates already route any destructive op to the same confirm by category, so this is just a new shape plus wording in the shared classifier. There's still no MCP dataset-delete tool (that's galaxyproject#228); when one lands it should get an entry in the same catalog.
A review pass flagged gaps in the dataset gate's coverage and honesty, fixed here:
- Also catch the singleton DELETE /api/datasets/{id} and batch /api/datasets
routes, not just /api/histories/{id}/contents/{id}.
- Treat the purge values Galaxy actually accepts as truthy (1/yes/on, not only
"true"), and treat an unknowable value -- a shell variable, or a body read
from a file we can't inspect -- as a purge, so an irreversible purge is never
described as "recoverable". An explicit purge=false stays a soft delete.
- Check the most-severe target first, so a whole-history delete in a command
that also touches a dataset URL isn't concealed behind the dataset warning.
- Handle the modern typed contents routes: /contents/datasets/{id} extracts the
real id (not the literal "datasets"), and /contents/dataset_collections/{id}
is classified as a collection -- with wording that it removes all of its
datasets -- instead of a single dataset.
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 free
to 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.
Follow-up to #338 (merged in #344).
#344 gated whole-history delete/purge but deliberately scoped dataset-level deletes out -- the raw-curl guardrail skipped the
/contents/sub-path so it couldn't mislabel a dataset delete as "entire history". A dataset purge is still irreversible data loss, just a smaller blast radius, so this extends the same classifier to it.What changed
isGalaxyDestructiveCurlnow classifies acurl/wgetDELETE against/api/histories/{id}/contents/{dsid}: a plain delete reads asdataset-delete(recoverable soft-delete), and a purge (?purge=truein the query or the request body) asdataset-purge(irreversible), each with honest dataset-scoped wording. IDs are surfaced only when literal (a$VARstill matches but isn't echoed as a fake id).No gate/policy/web changes
Both
tool_callgates already route any destructive op to the same confirmation by category (galaxy:destructive), so this is just a new shape + wording in the shared classifier -- the classifier was built for exactly this kind of extension.Scope
Covers the raw-curl path, which is the only dataset-delete vector today -- there's no MCP dataset-delete tool yet (#228). When one ships, it should get a single entry in the same catalog. Collection deletes and
/api/datasets/{id}are not covered here.Tests
Classifier unit tests for dataset delete/purge (query + body forms), shell-variable ids, and dataset-scoped wording (asserting it does NOT say "entire history" for a dataset). Full suite green; root + app typecheck clean.