Closed
Address review comments on bulk skeleton endpoints#8
Conversation
…n validation, RID limits, fast-path, remove unused param, add tests Co-authored-by: fcollman <782341+fcollman@users.noreply.github.com>
Co-authored-by: fcollman <782341+fcollman@users.noreply.github.com>
Copilot
AI
changed the title
[WIP] Address reviews on current pull request
Address review comments on bulk skeleton endpoints
Mar 14, 2026
Contributor
|
I want the server to control what the expiration of the gcs token is, and have it default to 1 hour for now but have that parameter controlled in the config |
fcollman
reviewed
Mar 14, 2026
Contributor
There was a problem hiding this comment.
I want the expiration minutes to be a server side config
- Add SKELETON_TOKEN_EXPIRATION_MINUTES=60 to BaseConfig in config.py - Re-add expiration_minutes param to get_skeleton_token_by_datastack_and_rids() sourced from server config (not client request body) - Cap reported token expiry to min(credential_expiry, now + expiration_minutes) with proper timezone-aware datetime comparison - api.py reads SKELETON_TOKEN_EXPIRATION_MINUTES from Flask app config and passes it down to the service layer Co-authored-by: fcollman <782341+fcollman@users.noreply.github.com>
…vice Server-side config for GCS token expiration
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
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.
Addresses all 8 Copilot review comments on PR #7 (bulk endpoints and auth tokens).
Key changes
service.pyskeletons[rid]→skeletons[str(rid)]for JSON consistency with the token endpointget_skeleton_by_datastack_and_rid()call with direct_retrieve_skeleton_from_cache(params_cached, "h5_mpsk")+ local format conversion, bypassing per-RID CAVEclient validation (the stated goal of this endpoint)skeleton_versiontoHIGHEST_SKELETON_VERSIONwith log message and documented in docstring. Addedskeleton_versionto response so callers see what they gotMAX_BULK_CACHED_SKELETONStruncation, matching the cached bulk endpointexpiration_minutes: Parameter was accepted but never used to control token lifetime—downscoped tokens inherit source credential expiryroles/storage.objectVieweris the narrowest built-in GCS role usable with Credential Access Boundariesapi.pyMAX_BULK_CACHED_SKELETONSimport is now used: truncates RID list in token endpointpost()expiration_minutesplumbing from handlertests/test_skeletonservice.pytest_get_cached_skeletons_bulk_cache_hit— cache hit vs miss handlingtest_get_cached_skeletons_bulk_rid_truncation— verifies truncation atMAX_BULK_CACHED_SKELETONStest_get_cached_skeletons_bulk_string_keys— verifies dict keys are stringsOriginal prompt
💬 Send tasks to Copilot coding agent from Slack and Teams to turn conversations into code. Copilot posts an update in your thread when it's finished.