feature/globus by bwalsh · Pull Request #251 · calypr/git-drs

bwalsh · 2026-08-05T19:52:29Z

Summary

Adds Globus Transfer support for hydrating DRS objects into the Git LFS cache.

Selects usable Globus access methods while falling back per object when unavailable.
Resolves DRS access_id values correctly.
Authenticates with the Globus Transfer API.
Submits checksum-synchronized transfers using valid Transfer API payloads.
Writes directly to the repository’s .git/lfs/objects cache path.
Polls recoverable INACTIVE tasks until completion or cancellation.
Verifies downloaded size and SHA-256 before exposing cached content.
Rejects unsupported Globus remote-auth configuration.
Updates user and developer documentation.

Configuration

The destination Globus collection must expose the repository root.

  export GIT_DRS_GLOBUS_TRANSFER_TOKEN='<token>'
  export GIT_DRS_GLOBUS_DESTINATION_COLLECTION='<collection-id>'

To prefer Globus when available:

  export GIT_DRS_ACCESS_METHOD=globus

Testing

  go test ./...

All tests pass.

Reviewer note

Please pay particular attention to the documentation changes:

ADR: docs/access-method-selection-and-authentication.md
- Per-object access-method selection
- Preference and fallback behavior
- Separation of transfer selection from authentication
- Mixed Globus and non-Globus pulls
Developer architecture: docs/globus-notes-high-level.md
- Globus ownership boundaries
- Destination collection and LFS cache mapping
- Client-managed transfer workflow
User documentation: docs/globus.md
- Required environment variables
- Destination collection setup
- Globus preference behavior
- Authentication and troubleshooting
Troubleshooting: docs/troubleshooting.md
- Globus token and collection checks
- LFS cache visibility requirements

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: f8af218063

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

chatgpt-codex-connector · 2026-08-05T19:56:52Z

+	if method.AccessUrl != nil && strings.TrimSpace(method.AccessUrl.Url) != "" {
+		return &drsapi.AccessURL{Headers: method.AccessUrl.Headers, Url: strings.TrimSpace(method.AccessUrl.Url)}, nil


Keep resolving provider access URLs through DRS

When a record was created by add-url/push with an inline provider URL, this branch returns the raw s3:///gs:// value stored in AccessMethod.AccessUrl (see internal/drsobject/object.go:125-130 and docs/adding-s3-files.md:7-11). DownloadResolvedToPath then hands that URL to Syfon's GenericDownload, whose implementation only treats file paths specially and otherwise performs an HTTP GET (https://github.com/calypr/syfon/blob/development/client/transfer/http.go#L676-L804), so normal provider-backed pulls fail with an unsupported protocol scheme instead of asking the DRS server for a presigned URL. Restrict direct returns to schemes the downloader handles, or continue resolving storage-provider URLs through DRS.

Useful? React with 👍 / 👎.

@matthewpeterkort See #252

bwalsh · 2026-08-05T20:48:38Z

@chatgpt-codex-connector review

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: fe920e3e84

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

matthewpeterkort

This PR introduces alot of engineering choices to overengineer around a fundamentally lacking DRS schema. Instead of overengineering, DRS should be changed to support this new multiple auth multiple access methods real world use case.

Below is an attempt at finding faults and proposing solutions to the current DRS schema. This is a first attempt that probably needs some refining.
#253

matthewpeterkort · 2026-08-06T16:30:08Z

+1. a single `DrsObject` with several possible access methods, such as `https`, `s3`, and `globus`; and
+2. a collection of `DrsObject` values whose available access methods and authorization requirements differ from object to object.
+
+DRS does not define a priority among access method types. An access method `type` identifies how the bytes can be transferred; it does **not** uniquely identify how the transfer must be authenticated.


this does not really flow. You go from taling about basic drs concepts into this "access method" concept. Perhaps a paragraph talking about access methods first is needed here.

matthewpeterkort · 2026-08-06T16:31:04Z

+
+| Interaction | DRS mechanism |
+| --- | --- |
+| Retrieve `DrsObject` metadata | Authorization discovery for the object endpoint, including `OPTIONS /objects/{object_id}` |


yeah like this section probably goes before the paragraphs above it

matthewpeterkort · 2026-08-06T16:32:35Z

+4. select the highest-priority usable method according to client configuration; and
+5. allow the user to override automatic selection explicitly.
+
+Each transfer handler owns the protocol-specific logic needed to resolve and execute its transfer. Authentication is determined from the DRS authorization information and the selected handler's capabilities, not inferred solely from `AccessMethod.type`.


maybe give an example here?? "Authentication is determined from the DRS authorization information and the selected handler's capabilities, not inferred solely from AccessMethod.type" this is pretty important and appears to be glossed over here.

matthewpeterkort · 2026-08-06T16:35:05Z

+DRS defines no preferred ordering, so git-drs will provide a configurable client-side priority. For example:
+
+```yaml
+access_method_priority:


this only matters if there is more than 1 access method provided right?

matthewpeterkort · 2026-08-06T16:37:30Z

+GIT_DRS_ACCESS_METHOD=globus git drs pull
+```
+
+The preference is evaluated per object. If an object does not advertise the


If an access method is configured then it should be useable. If it cannot be used to create a download/upload then it should not be tried. Like having globus at the top of the priority and then the access method doesn't produce a object, can quickly lead to performance losses from repeatedly trying access methods that do not lead to a successful result.

matthewpeterkort · 2026-08-06T16:51:30Z

+
+For Globus in particular, objects that resolve to compatible Globus endpoints and authorization contexts should be grouped into one batch transfer where possible.
+
+DRS bulk-access authorization imposes an additional constraint: a bulk request may require a common passport or bearer token for the requested objects. git-drs must therefore partition heterogeneous objects into authorization-compatible groups or resolve them individually rather than assuming a single credential applies to the entire collection.


well a dynamic mapping would fix this problem. This quickly becomes more complexity than anyone wants to deal with.

You say drs doesn't support a fixed mapping but this is because it assumes that one auth will be used for all the data sources. This is not the case in the real world. I suspect this a DRS shortcoming and folllowing the drs spec here will prove to be a detriment in the long run.

matthewpeterkort · 2026-08-06T16:53:27Z

+
+### Positive
+
+- Adding Globus does not require special authentication semantics in the DRS core.


Sure. but Globus has its own auth. I'm not sure if an auth abstraction is really what is needed here.

matthewpeterkort · 2026-08-06T17:27:29Z

+Export these environment variables before running `git drs pull`:
+
+```bash
+export GIT_DRS_GLOBUS_TRANSFER_TOKEN='<globus-transfer-api-access-token>'


Just use globus login command and then read from the globus config file? Messing with env vars is making it more difficult for the user than it needs to be. This is not a script, this is a CLI.

matthewpeterkort · 2026-08-06T17:27:52Z

+Verify the token before pulling:
+
+```bash
+git drs auth globus


this is good

matthewpeterkort · 2026-08-06T17:32:26Z

+		return nil
+	}
+	methods := *obj.AccessMethods
+	preferred := strings.TrimSpace(strings.ToLower(os.Getenv("GIT_DRS_ACCESS_METHOD")))


this is pretty hackish. This needs to be scallable across more than just a CLI. There should be a default access method that is intelligently configured depending on if there is more than one access method specified. Like ideally the files on Globus eventually make it to synapse aws s3 bucket once the paper is published, etc so certain access methods would have higher priority over others ,etc.

bwalsh added 3 commits August 4, 2026 12:06

initial notes

025cee8

Add Globus API authentication and transfer support

b241fa1

Adds Globus transfers for DRS downloads

f8af218

chatgpt-codex-connector Bot reviewed Aug 5, 2026

View reviewed changes

bwalsh added 4 commits August 5, 2026 13:37

Retry skipped objects on push

9402f45

Fall back to DRS access IDs

fefa977

Honor usable DRS access methods

1b2c914

Harden Globus transfer handling

fe920e3

chatgpt-codex-connector Bot reviewed Aug 5, 2026

View reviewed changes

Comment thread internal/transfer/download.go Outdated

bwalsh mentioned this pull request Aug 5, 2026

improvement/syfon/GenericDownload #252

Open

bwalsh added 8 commits August 5, 2026 14:45

Normalize GA4GH SHA-256 checksums

2c69032

Skip unsupported direct access URLs

ef5ba35

Document transport-neutral Globus workflow

88423d8

Remove partial Globus downloads

e8b7eff

Improve access method check error msgs

4b64436

Resolve S3 access methods by ID

b5d3058

Improve documentation for globus drs.AccessMethod

abcf3e8

clarify GIT_DRS_GLOBUS_DESTINATION_COLLECTION

d427f53

matthewpeterkort requested changes Aug 6, 2026

View reviewed changes

		if method.AccessUrl != nil && strings.TrimSpace(method.AccessUrl.Url) != "" {
		return &drsapi.AccessURL{Headers: method.AccessUrl.Headers, Url: strings.TrimSpace(method.AccessUrl.Url)}, nil


		For Globus in particular, objects that resolve to compatible Globus endpoints and authorization contexts should be grouped into one batch transfer where possible.

		DRS bulk-access authorization imposes an additional constraint: a bulk request may require a common passport or bearer token for the requested objects. git-drs must therefore partition heterogeneous objects into authorization-compatible groups or resolve them individually rather than assuming a single credential applies to the entire collection.


		### Positive

		- Adding Globus does not require special authentication semantics in the DRS core.

Uh oh!

Conversation

bwalsh commented Aug 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Configuration

Testing

Reviewer note

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

chatgpt-codex-connector Bot Aug 5, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bwalsh commented Aug 5, 2026

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

Uh oh!

matthewpeterkort left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

bwalsh commented Aug 5, 2026 •

edited

Loading