[matthewpeterkort]

No description provided.

[chatgpt-codex-connector]

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review	Status	Commit	Review trigger
📝 Code Review	✅ Completed 2026-10-07T21:58:22.335186Z	bb6c628	PR opened

ℹ️ 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" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: bb6c6288f5

ℹ️ 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]

Resolve push uploads from the configured LFS cache

When lfs.storage is configured, clean now stores the payload under this resolved objDir, but internal/pushsync/register.go:127 still searches only .git/lfs/objects. The push inventory comes from Git blobs and marks these entries as pointers, so resolveUploadSourcePath will not fall back to the worktree and silently skips the upload; the Git push can therefore succeed while the newly registered DRS object has no payload. Pass the resolved LFS root through the push path as well.

Useful? React with 👍 / 👎.

[bwalsh]

Codex review

Must fix

1. A resumed download can accept corrupted data (internal/resolver/resolver.go:82).
   • What this is: Downloads resume from a persistent partial file.

   • Problem: If the resolver supplies no supported checksum, the code trusts the existing bytes and checks only the final size. A damaged partial followed by a valid range response can be promoted
   and hydrated.

   • Fix: Resume only when a supported checksum is available; otherwise restart the download.

   • If skipped: A DRS URI pointer without a checksum can yield incorrect file content without an error.

2. Concurrent downloads share one partial file (internal/resolver/resolver.go:82).
   • What this is: Every download to the same cache destination opens the same partial path.
   • Problem: Two Git operations can truncate or write that file at the same time, or one can rename it while the other still uses it.
   • Fix: Serialize downloads for a destination before opening its partial file.
   • If skipped: Concurrent pull or filter operations can fail or interfere with each other.

Should fix

3. An early integration failure can leave an emulator running (tests/terra-drs-hub/run.sh:73).
   • What this is: The cleanup trap is installed after both Docker emulators start.
   • Problem: If the storage emulator fails to start, the script exits while the Firestore container remains running.
   • Fix: Install the trap before starting either emulator, with cleanup that handles services that have not started.
   • If skipped: Failed local runs can leave containers behind.

Verdict: fix 1 and 2 first. The focused Go package tests, shell syntax check, and diff whitespace checks passed.

[bwalsh]

Syfon Backend E2E / Syfon Docker E2E (add-url) (pull_request)

@matthewpeterkort

The change that caused it is the add-url CLI refactor in PR #266.

Specifically, in cmd/addurl/main.go, the command signature was changed from the old single-object form:

• old: git drs add-url <object-url-or-key> [path]

to the new manifest form:

• new: git drs add-url <manifest.tsv>

and the argument validation was tightened to:

• Args: cobra.ExactArgs(1)
• RunE: runManifest

That means the legacy invocation used by the failing E2E test:

• git drs add-url s3://.../object data/from-bucket.bin --sha256 ...

now violates the new contract and fails with:

• Error: accepts 1 arg(s), received 2

So the root cause was not the workflow itself; it was the breaking change in the PR that redesigned git drs add-url to a manifest-only API and left the integration test still calling the old 2-arg form.

[matthewpeterkort]

yes that makes sense. the add url test will need to be modified to support this new workflow.