Add Terra/AnVIL ADR, TDD guide, and acceptance tests for DRS URI support#247
Add Terra/AnVIL ADR, TDD guide, and acceptance tests for DRS URI support#247bwalsh wants to merge 16 commits into
Conversation
|
Expected behavior: |
![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: 953f79d7b8
ℹ️ 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".
|
tests now pass |
There was a problem hiding this comment.
AI aided review summary:
This is a useful first design step, but the core abstraction should be a provider-neutral DRS URI resolver plus pluggable authentication. Terra should be the first authentication/service adapter, not the object identity model. The implementation needs to distinguish URI resolution, service authentication, access-method selection, download, and checksum verification. In particular, a DRS URI cannot be treated as a SHA256 object ID merely because it needs a local cache path. The next implementation step should establish that generic pipeline, then add Terra authentication behind it.
Technical design remains too vague in the areas that determine whether the approach will scale beyond Terra. I’m not asking for a broad refactor in this PR, but the PR should define the adapter boundaries clearly enough that the next implementation work has a concrete shape.
places to go further into depth on:
-
URI parsing and resolution:hostname-based drs://host/id
a. compact-identifier drs://namespace:id
b. resolver registry lookup and caching -
authentication:anonymous/public access
a. bearer tokens
b. Terra/Google ADC
c. GA4GH Passport or future mechanisms -
DRS interaction:fetching object metadata
a. selecting access_url versus access_id
b. handling access-method-specific headers -
adapter interfaces:resolver adapter
a. auth provider
b. DRS client/service adapter
c. byte downloader -
error and security policy:unknown compact prefixes
a. untrusted resolver responses
b. checksum mismatches
c. auth failure versus object-not-found
Checksum/hash/identifier resolution is also huge here. Currently we just use sha256 as the identifier in the local cache, but I suspect that drs URIs, or other types of checksums should be supported to make this more extensible. It is unreasonable to expect users to download files from remote just to compute a sha256. Whatever hash that is provided in the remote should be used as the sha256.
a concrete “next implementation boundary” section showing the expected data flow and interfaces + more technical details that answer the above elaboration questions would be useful
|
|
||
| func init() { | ||
| Cmd.Flags().StringVarP(&remote, "remote", "r", "", "target remote DRS server (default: default_remote)") | ||
| Cmd.Flags().StringVar(&remoteType, "remote-type", "", "resolver remote type for DRS references (for example: terra)") |
There was a problem hiding this comment.
Ideally, you want the user not have to define this as a feature flag because that is not very scalable. Instead you use identifiers.org or something similar to use the drs compact id lookup to resolve the drs server.
|
|
||
| - Git-compatible pointer workflows. | ||
| - `git drs track` for tracked data path patterns. | ||
| - `git drs pull -I <pattern>` for selective hydration. |
There was a problem hiding this comment.
The current pull validation compares the downloaded SHA256 to the pointer’s oid. That works for oid sha256:..., but cannot work for oid drs://... without changing the verification model.
|
In particular, a DRS URI cannot be treated as a SHA256 object ID merely because it needs a local cache path. The next implementation step should establish that generic pipeline, then add Terra authentication behind it.
Not sure I follow.
Can you explain/restate?
…On Fri, Jul 10, 2026, 4:57 PM Matthew Peterkort ***@***.***> wrote:
***@***.**** requested changes on this pull request.
AI aided review summary that I agree with:
This is a useful first design step, but the core abstraction should be a
provider-neutral DRS URI resolver plus pluggable authentication. Terra
should be the first authentication/service adapter, not the object identity
model. The implementation needs to distinguish URI resolution, service
authentication, access-method selection, download, and checksum
verification. In particular, a DRS URI cannot be treated as a SHA256 object
ID merely because it needs a local cache path. The next implementation step
should establish that generic pipeline, then add Terra authentication
behind it.
Technical design remains too vague in the areas that determine whether the
approach will scale beyond Terra. I’m not asking for a broad refactor in
this PR, but the PR should define the adapter boundaries clearly enough
that the next implementation work has a concrete shape.
places to go further into depth on:
1.
URI parsing and resolution:hostname-based drs://host/id
a. compact-identifier drs://namespace:id
b. resolver registry lookup and caching
2.
authentication:anonymous/public access
a. bearer tokens
b. Terra/Google ADC
c. GA4GH Passport or future mechanisms
3.
DRS interaction:fetching object metadata
a. selecting access_url versus access_id
b. handling access-method-specific headers
4.
adapter interfaces:resolver adapter
a. auth provider
b. DRS client/service adapter
c. byte downloader
5.
error and security policy:unknown compact prefixes
a. untrusted resolver responses
b. checksum mismatches
c. auth failure versus object-not-found
Checksum/hash/identifier resolution is also huge here. Currently we just
use sha256 as the identifier in the local cache, but I suspect that drs
URIs, or other types of checksums should be supported to make this more
extensible. It is unreasonable to expect users to download files from
remote just to compute a sha256. Whatever hash that is provided in the
remote should be used as the sha256.
I’d suggest asking the author to add a concrete “next implementation
boundary” section showing the expected data flow and interfaces. That would
keep this PR focused while making the future Terra and multi-provider work
technically actionable.
------------------------------
In cmd/addref/add-ref.go
<#247 (comment)>:
> @@ -61,4 +62,5 @@ var Cmd = &cobra.Command{
func init() {
Cmd.Flags().StringVarP(&remote, "remote", "r", "", "target remote DRS server (default: default_remote)")
+ Cmd.Flags().StringVar(&remoteType, "remote-type", "", "resolver remote type for DRS references (for example: terra)")
Ideally, you want the user not have to define this as a feature flag
because that is not very scalable. Instead you use identifiers.org or
something similar to use the drs compact id lookup to resolve the drs
server.
------------------------------
In docs/terra-anvil-drs-download-gap-adr.md
<#247 (comment)>:
> +
+Gregor project users want a Git repository that stores project structure, manifests, analysis code, and `git-drs` pointer files while leaving controlled-access payload bytes on AnVIL. Users should be able to clone normal Git history, review which data objects are referenced, authenticate through AnVIL/Terra access controls, and hydrate only selected paths with commands such as:
+
+```bash
+git drs pull -I "data/cohort-a/**"
+```
+
+Terra/AnVIL DRS downloads typically start from stable DRS URIs and use Terra-compatible authentication and resolution tooling to obtain authorized access URLs. A repository bootstrap workflow also needs to query AnVIL DRS metadata and generate pointer files and manifests deterministically.
+
+## Current state
+
+`git-drs` already provides several useful building blocks:
+
+- Git-compatible pointer workflows.
+- `git drs track` for tracked data path patterns.
+- `git drs pull -I <pattern>` for selective hydration.
The current pull validation compares the downloaded SHA256 to the
pointer’s oid. That works for oid sha256:..., but cannot work for oid
drs://... without changing the verification model.
—
Reply to this email directly, view it on GitHub
<#247?email_source=notifications&email_token=AAALVQBDYRUH2QKB7N7CPN35EF7HTA5CNFSNUABKM5UWIORPF5TWS5BNNB2WEL2QOVWGYUTFOF2WK43UKJSXM2LFO4XTINRXGU3DENZWHE22M4TFMFZW63VGMF2XI2DPOKSWK5TFNZ2KYZTPN52GK4S7MNWGSY3L#pullrequestreview-4675627695>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAALVQBPK6OR3YU5DEN64X35EF7HTAVCNFSNUABFKJSXA33TNF2G64TZHM4TMMZQHAYDENZRHNEXG43VMU5TIOBUGEZDKMJYGI22C5QC>
.
Triage notifications, keep track of coding agent tasks and review pull
requests on the go with GitHub Mobile for iOS
<https://github.com/notifications/mobile/ios/AAALVQEPJHRDFGYOJMKT2DT5EF7HTA5CNFSNUABKM5UWIORPF5TWS5BNNB2WEL2QOVWGYUTFOF2WK43UKJSXM2LFO4XTINRXGU3DENZWHE22M4TFMFZW63VGMF2XI2DPOKSWK5TFNZ2KUZTPN52GK4S7NFXXG>
and Android
<https://github.com/notifications/mobile/android/AAALVQEDPQ5ZNPSWB3ZKMVD5EF7HTA5CNFSNUABKM5UWIORPF5TWS5BNNB2WEL2QOVWGYUTFOF2WK43UKJSXM2LFO4XTINRXGU3DENZWHE22M4TFMFZW63VGMF2XI2DPOKSWK5TFNZ2K4ZTPN52GK4S7MFXGI4TPNFSA>.
Download it today!
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
|
A DRS URI identifies a file, but it is not the file’s checksum. The way that git-drs is engineered currently you need a hash to do anything file related. |
bwalsh
changed the title
|
Updated the add-ref documentation to state that the client resolves drs://... URIs against the source authority/resolver using source-appropriate credentials, and only uses the primary remote as an explicit resolver/proxy. Corrected the sequence diagram so add-ref talks to the Source DRS authority/resolver by default, with a separate optional primary-remote proxy path. Updated the immediate add-ref state guidance to clarify that primary remote state is not required and source DRS hydration should use the source authority unless the primary remote is explicitly configured as a proxy. Changed git drs add-ref resolution to call resolveAddRefObject instead of always resolving through the selected primary remote client. Added source-authority resolution logic that matches configured remotes by the DRS URI authority, uses that source remote when available, and falls back to direct source authority resolution when the primary remote does not match the DRS URI authority. Added a test proving that add-ref contacts the configured source authority with source credentials and does not contact the primary remote for a foreign drs://source/object URI. |
|
Added user-facing documentation for multiple git-drs remote roles, clarifying that the default/explicit command remote is the primary remote for repository-scoped operations, while a drs://... URI identifies a source DRS authority/resolver. The docs now explicitly state that the primary remote does not act as a proxy for other source DRS servers. Clarified the pointer-file add-ref discussion so a DRS URI pointing at another authority leaves primary remote state unchanged and remains valid because hydration uses the preserved source identity directly. Updated add-ref resolution so DRS URIs are parsed into a source endpoint first; configured remotes are used only when their endpoint host matches the DRS URI host, otherwise resolution goes directly to the source endpoint instead of falling back to the primary remote. Added source-resolution helpers and a test seam for the direct source getter used when no configured remote matches the DRS URI host. Added a regression test proving that an unconfigured source authority resolves via the source endpoint and object ID, and that the primary remote is not contacted |
Reviewer Guide: Remotes, Credentials, and StatePlease review this PR with special attention to the multi-remote model and 1. How remotes are configuredPlease check whether the documentation clearly explains that a repository can have multiple Useful entry points:
Links: 2. How users provide credentials for those remotesPlease check whether the docs and source make it clear which credentials are used for each remote type. Useful entry points:
Links: 3. How state is maintainedPlease review whether the documentation correctly distinguishes:
For
Implementation entry points:
Links: |
|
Updated ping health checks to carry optional service-info response data, and print it as service-info: ... after a successful health check. Changed the Terra service-info health probe to return the response body from the /ga4gh/drs/v1/service-info endpoint, trimming whitespace while preserving the returned content for display. Added regression coverage confirming git drs ping includes Terra service-info output from the mocked service-info endpoint. Added a focused test confirming the command prints service-info data when the health probe returns it. Updated the Terra integration test helper call for the new service-info-returning function signature. |
2 participants
Summary
This PR documents and codifies the first increment of Terra/AnVIL DRS support for
git-drs.It adds:
terraremote from Git config;git drs add-ref;git-drspointer files withoid drs://...;The tests are intentionally red. They are meant to drive the next implementation step rather than validate current runtime behavior.
Motivation
Terra/AnVIL users need to reference controlled-access DRS objects in Git without committing payload bytes. The desired workflow is to store reviewable pointer files and manifests in Git, then hydrate selected files through AnVIL/Terra authorization.
The current
git-drsimplementation already has useful building blocks, including pointer workflows, selective hydration,add-ref, Gen3/Syfon remotes, and local DRS metadata/cache plumbing. However, those pieces do not yet compose into a complete Terra/AnVIL workflow.This PR frames the remaining work as an incremental provider/identity extension rather than a wholesale rewrite.
The key missing pieces are:
terraremote mode;add-refbehavior;Changes
Documentation
Adds
docs/terra-anvil-drs-download-gap-adr.md.git drs remote add terra ...shape.add-refdirection.oid sha256:<derived-local-oid>pointers and directgit-drspointers usingoid drs://....Adds
docs/terra-anvil-tdd-tests.md.Adds this PR-description Markdown file at
docs/terra-anvil-pr-description.md.Acceptance tests
Adds
cmd/addref/terra_addref_acceptance_test.go.git drs add-refto expose a--remote-typeflag or equivalent resolver-selection mechanism for Terra references.Adds
internal/config/terra_remote_acceptance_test.go.LoadConfigandGetRemote("anvil")to recognize atype = terraremote and preserve the configured endpoint.Adds
internal/lfs/terra_pointer_acceptance_test.go.git-drsto recognize a pointer withversion https://calypr.github.io/spec/v1andoid drs://....Expected current test behavior
The new acceptance tests are expected to fail until the corresponding Terra/AnVIL implementation is added.
Run the focused test group with:
go test -timeout 30s ./internal/lfs ./internal/config ./cmd/addrefExpected current failure categories:
internal/lfs: DRS URI pointer parsing and cache-key derivation are not implemented yet.internal/config:type = terraremotes are not parsed into a concrete DRS remote yet.cmd/addref:--remote-typeis not exposed yet.Reviewer notes
This PR intentionally does not implement Terra/AnVIL runtime support yet. It establishes:
Once Terra/AnVIL implementation work lands, these intentionally failing tests should become passing regression tests.