feature/globus - #251
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 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".
| if method.AccessUrl != nil && strings.TrimSpace(method.AccessUrl.Url) != "" { | ||
| return &drsapi.AccessURL{Headers: method.AccessUrl.Headers, Url: strings.TrimSpace(method.AccessUrl.Url)}, nil |
There was a problem hiding this comment.
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 👍 / 👎.
There was a problem hiding this comment.
|
@chatgpt-codex-connector review |
There was a problem hiding this comment.
💡 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
left a comment
matthewpeterkort
left a comment
There was a problem hiding this comment.
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
| 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. |
There was a problem hiding this comment.
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.
|
|
||
| | Interaction | DRS mechanism | | ||
| | --- | --- | | ||
| | Retrieve `DrsObject` metadata | Authorization discovery for the object endpoint, including `OPTIONS /objects/{object_id}` | |
There was a problem hiding this comment.
yeah like this section probably goes before the paragraphs above it
| DRS defines no preferred ordering, so git-drs will provide a configurable client-side priority. For example: | ||
|
|
||
| ```yaml | ||
| access_method_priority: |
There was a problem hiding this comment.
this only matters if there is more than 1 access method provided right?
|
Update for August 11, 2026 Implemented the Globus authentication and client access-method work discussed in this PR. Globus authentication and transfer
Client access-method policy
Documentation and validation
Commits:
|
|
Today’s commits implement the issue #254 client-policy design:
Key ownership boundary:
Verification completed:
The unrelated untracked |
Verify checksum-less add-url payloads hydrate successfully, persist the learned SHA-256 locally, and publish it to Syfon through an explicit push.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 298eb87819
ℹ️ 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".
Follow offset pagination for recursive directory imports and submit only one Globus transfer item per cache destination.
Keep authoritative remote metadata when cloned placeholders have no local sidecar, and recognize the placeholder checksum marker during single-file Globus verification.
Overlay clone-local endpoints onto shared generic remotes without discarding provider, authentication, scope, or selection policy.
Secure token files without chmodding caller-supplied parent directories.
Stage the Globus add-url page and generated presentation in MkDocs input so strict Pages builds resolve every published link.
Require an HTTP host so access planning resolves a valid access ID or falls back to another advertised method.
Escape question marks in the Globus path before URL parsing so documented single-character wildcard patterns remain intact.
Track shared-policy provenance through local overlays and stop the remote remove command before it mutates configuration.
…ll DRS metadata fields from internal records; globus: validate effective credential in status\n\nFixes:\n- Treat per-method access resolution errors as non-fatal so later methods are tried (AnVIL gs:// then HTTPS fallback).\n- Map Description, Version, CreatedTime, UpdatedTime when converting internal records to DRS objects to avoid dropping metadata.\n- Check the effective Globus credential in 'globus status' by validating the environment or stored token (use Check with environment enabled).\n\nCo-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Commits
|
…e\n\nStop tracking the accidental syfon submodule pointer and add syfon/ to .gitignore to avoid showing local modifications while keeping the working tree intact.\n\nCo-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 493974be27
ℹ️ 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".
Resolve review feedback on #251 by rejecting strict access methods the read-only AnVIL resolver cannot enforce, honoring policy precedence, and treating zero-byte cache entries as complete.
|
@codex review |
|
Codex Review: Didn't find any major issues. More of your lovely PRs please. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
2 participants
This branch is a large feature/bugfix slice built on top of
development, with the main themes being:Globus integration and transfer workflow expansion
internal/globusauth/*,internal/transfer/globus.go,cmd/addurl/globus_recursive.go,cmd/auth/globus.go.DRS access-method and metadata robustness fixes
internal/resolver/resolver.go,internal/lookup/lookup.go,internal/transfer/access.go.Shared remote / config safety
internal/config/*,cmd/remote/remove.go.Documentation and test coverage
The most relevant recent commits for your immediate review are:
dd4d5c9— fix resolver fallback + metadata preservation + effective Globus status validation6bbb054— reject removal of shared remotes4c1675a— preserve raw Globus question wildcards71e4720— reject hostless HTTP access URLsThe branch also includes a commit that cleaned up the accidental syfon gitlink and ignored the local syfon working tree (
8e53b7c), which is unrelated to runtime behavior.Reviewer notes
Please pay particular attention to the documentation changes:
ADR: docs/access-method-selection-and-authentication.md
Developer architecture: docs/globus-notes-high-level.md
User documentation: docs/globus.md
Troubleshooting: docs/troubleshooting.md