Add Phase 2 remote bundle jobs (server + client) - #129
Merged
Conversation
Introduce Phase 2 remote-job flow: server-side bundling and Openbin upload, plus client support to use those bundles with byte-range resume. Key changes: - internal/git: Add CreateBundle to clone by layers, complete history and produce a .bundle file. - internal/http: Add v2 /jobs endpoints and worker (presign PUT to Openbin, confirm) to create/store bundles (jobs2.go) and register routes (router.go). - internal/jobs: Add client-side bundle clone flow (runCloneBundle, download with Range, sha256 verification, materialize repo), fallback to layered clone if server lacks /v2/jobs; include tests for resume/hash behavior (clone_bundle.go, clone_bundle_test.go). - web/index.html: adjust layout / right sidebar and sponsor block. This enables creating bundles server-side, serving them via CDN (Openbin/Filebase), and robust client downloads with resume and integrity checks.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Contributor
|
Warning Review limit reached
Next review available in: 12 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (19)
📝 WalkthroughWalkthroughAdds remote Git bundle jobs with protected API routes, Openbin uploads, resumable client downloads, checksum validation, and layered-clone fallback. It also removes obsolete sidebar metrics and sponsor markup and sponsor width overrides. ChangesRemote bundle jobs
Web sidebar cleanup
Estimated code review effort: 5 (Critical) | ~90+ minutes Sequence Diagram(s)sequenceDiagram
participant Client as runCloneBundle
participant API as Remote job API
participant Openbin
participant Repository as Local repository
Client->>API: Create and poll bundle job
API->>Openbin: Upload generated bundle
Openbin-->>API: Return bundle metadata
API-->>Client: Return ready job and checksum
Client->>Openbin: Resume bundle download
Client->>Repository: Materialize bundle and set origin
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
gitgost-anonymous
previously approved these changes
Aug 1, 2026
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 13
🧹 Nitpick comments (4)
internal/git/bundle.go (1)
103-114: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winDo not parse
CombinedOutputas a git value.
gitOutputmerges stderr into stdout.revCountcallsstrconv.Atoion that text, and Line 52 uses it as the default branch name. Any git warning on stderr (for example a hint or a redirect notice) then produces a wrong commit count or a wrong branch name. Capture stdout and stderr separately.♻️ Proposed refactor
func gitOutput(dir string, args ...string) (string, error) { cmd := exec.Command("git", args...) cmd.Dir = dir - out, err := cmd.CombinedOutput() - if err != nil { - if msg := strings.TrimSpace(string(out)); msg != "" { - return string(out), fmt.Errorf("%w: %s", err, msg) - } - } - return string(out), err + var stderr bytes.Buffer + cmd.Stderr = &stderr + out, err := cmd.Output() + if err != nil { + if msg := strings.TrimSpace(stderr.String()); msg != "" { + return string(out), fmt.Errorf("%w: %s", err, msg) + } + } + return string(out), err }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/git/bundle.go` around lines 103 - 114, Update gitOutput to capture stdout and stderr separately instead of using CombinedOutput, preserving stdout as the returned git value while incorporating stderr into errors only when the command fails. Ensure callers such as revCount and the default-branch lookup receive clean stdout without warnings or notices.internal/http/jobs2.go (1)
159-177: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winReuse the computed size and hash instead of recomputing them.
runRemoteJobcomputessizeandhashon Lines 159-168.openbinUploadcomputes both again on Lines 197-204, which reads the whole bundle a second time. Lines 176-177 then overwrite the values thatopenbinUploadalready set. Pass the size and hash intoopenbinUpload.♻️ Proposed refactor
-func openbinUpload(bundlePath, filename string) (*remoteJobResult, error) { +func openbinUpload(bundlePath, filename, hash string, size int64) (*remoteJobResult, error) { @@ - hash, err := sha256File(bundlePath) - if err != nil { - return nil, fmt.Errorf("calcular sha256 del bundle: %w", err) - } - size, err := fileSize(bundlePath) - if err != nil { - return nil, fmt.Errorf("tamaño del bundle: %w", err) - }- result, err := openbinUpload(bundlePath, bundleFilename(job.URL)) + result, err := openbinUpload(bundlePath, bundleFilename(job.URL), hash, size) if err != nil { publish(rjFailed, "", err.Error(), nil) return } - result.Size = size - result.Sha256 = hash result.DefaultBranch = defaultBranchAlso applies to: 197-204
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/http/jobs2.go` around lines 159 - 177, Update runRemoteJob and openbinUpload to pass the already computed size and hash into openbinUpload, removing its duplicate file-size and SHA-256 calculations and the redundant result assignments in runRemoteJob. Preserve the existing upload behavior and error handling while ensuring the returned result uses the supplied values.internal/jobs/clone_bundle_test.go (1)
30-32: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReturn a guarded accessor instead of
*[]string.The handler appends to
rangesundermu, but Lines 202-207 read the slice through the returned pointer without the lock. Undergo test -racea late or retried request can trip the detector. Return a snapshot function that takes the same lock.♻️ Proposed refactor
-func fakeBundleServer(t *testing.T, bundlePath string) (*httptest.Server, *[]string) { +func fakeBundleServer(t *testing.T, bundlePath string) (*httptest.Server, func() []string) { @@ t.Cleanup(srv.Close) - return srv, &ranges + return srv, func() []string { + mu.Lock() + defer mu.Unlock() + return append([]string(nil), ranges...) + } }Also applies to: 66-66, 202-208
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/jobs/clone_bundle_test.go` around lines 30 - 32, Update the test helper around the shared ranges variable to return a snapshot accessor rather than a pointer to the slice. Have the accessor lock mu, copy the current ranges contents, unlock, and return the snapshot; update all callers, including the checks near lines 66 and 202-208, to use the accessor so every read is synchronized.internal/jobs/clone_bundle.go (1)
49-58: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAlign the origin URL and the default branch with the layered path.
Two contract gaps exist here:
runCloneLayeredprefersjob.Originand falls back to the effective URL.materializeClonealways uses the reconstructed https URL. A user who cloned with an scp-like URL gets a differentorigindepending on which path ran.- The server publishes
defaultBranchinremoteJobResult(internal/http/jobs2.goLine 52), butremoteJobBundleInfohas no matching field, so the value is dropped.git clone <bundle>then depends on the bundle carrying a usableHEAD. If it does not, the destination ends in a detached or unexpected branch.Prefer
job.Originwhen it is set, and use the reporteddefaultBranchto check out the expected branch.Also applies to: 356-370
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/jobs/clone_bundle.go` around lines 49 - 58, Update remoteJobBundleInfo and the bundle materialization flow to preserve the server’s defaultBranch value and check out that branch after cloning. Make materializeClone use job.Origin when set, falling back to the effective reconstructed URL, matching runCloneLayered’s origin behavior; retain the existing fallback when Origin is empty.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/git/bundle.go`:
- Around line 19-23: Thread a context.Context from runRemoteJob through
CreateBundle into runGit and gitOutput, creating it with context.WithTimeout in
runRemoteJob and using exec.CommandContext for every git subprocess. Enforce a
maximum accepted bundle size before the upload step, rejecting oversized bundles
and preserving cleanup/error propagation.
- Around line 64-72: Update shallowFile to support both repository layouts:
check the bare-repository marker at <dir>/shallow and the non-bare marker at
<dir>/.git/shallow, returning the contents of whichever exists. Preserve the
existing empty-string result when neither marker can be read so the no-progress
guard remains effective for mirror clones.
In `@internal/http/jobs2.go`:
- Around line 92-96: The POST /v2/jobs endpoint allows unbounded job creation
and lacks request limiting. In internal/http/jobs2.go lines 92-96, gate
runRemoteJob using a worker semaphore and return HTTP 429 when no slot is
available; in internal/http/router.go lines 303-311, add sizeLimitMiddleware()
and the existing per-IP limiter such as prCheckLimiter() to the v2 route group.
- Around line 331-347: Update validRepoURL to accept only the https scheme and
reject URLs containing embedded userinfo credentials before returning true;
preserve the existing supported-host and repository-path validation.
- Around line 131-152: Update the remote-job worker’s publish closure to retain
and propagate the temporary directory through every status update, using the
closure’s directory state rather than the original job copy. In
DeleteRemoteJobHandler, mark the job as cancelled without removing TmpDir while
the worker is running, so the worker’s existing defer os.RemoveAll(dir) performs
cleanup after termination.
- Around line 305-329: Update openbinUpload and openbinPut to pass the existing
fileSize(bundlePath) result into openbinPut, set req.ContentLength to that size,
and avoid chunked transfer encoding. Only set Content-Type when the presigned
URL was signed with that header, preserving the existing upload and response
error handling.
In `@internal/jobs/clone_bundle_test.go`:
- Around line 46-60: Fix the fake server’s Range parsing in the /bundle GET
handler: remove the trailing “-” from the byte range before parsing, and fail
loudly when parsing the range header is invalid instead of ignoring the error.
Preserve the partial-content response and write only data starting at the parsed
offset so TestDownloadBundleResumes exercises the actual resume path.
In `@internal/jobs/clone_bundle.go`:
- Around line 209-246: Update downloadBundle so a cached file whose size equals
size but whose hash does not match is removed before downloadWithRetry runs.
Update downloadRange to treat a partial offset at or beyond size as a restart
from zero, avoiding an invalid range request and allowing the download to
recover automatically.
- Around line 160-189: Update waitRemoteJob to enforce an overall polling
deadline and track consecutive errors from getRemoteJob, resetting the error
count after a successful response. Continue polling through a small bounded
number of transient errors, then return the final error; also return a clear
timeout error when queued/running polling exceeds the deadline while preserving
the existing ready, failed, and progress-update behavior.
- Around line 101-112: After materializeClone succeeds in the clone flow, delete
the cached bundle at cacheFile before marking progress complete; handle any
deletion failure through the function’s existing error-return pattern, while
preserving the current download and materialization behavior.
- Around line 133-158: Update createRemoteJob and the corresponding remote job
status request to send the GITGOST_API_KEY value in the X-Gitgost-Key header and
use an HTTP client configured with a finite timeout instead of
http.Post/http.Get defaults. Treat HTTP 401 and 403 like 404 by returning
errRemoteJobsUnsupported, preserving existing response handling and fallback
behavior.
In `@web/index.html`:
- Around line 1683-1685: In the stats note’s text, replace the grammatically
incorrect phrase “Data are storage in Zurich.” with “Data are stored in Zurich.”
- Around line 1679-1681: Update the element identified by ethicalmetrics-chart
to include a nameable semantic role, such as role="img", alongside its existing
aria-label so accessibility APIs expose the chart’s accessible name.
---
Nitpick comments:
In `@internal/git/bundle.go`:
- Around line 103-114: Update gitOutput to capture stdout and stderr separately
instead of using CombinedOutput, preserving stdout as the returned git value
while incorporating stderr into errors only when the command fails. Ensure
callers such as revCount and the default-branch lookup receive clean stdout
without warnings or notices.
In `@internal/http/jobs2.go`:
- Around line 159-177: Update runRemoteJob and openbinUpload to pass the already
computed size and hash into openbinUpload, removing its duplicate file-size and
SHA-256 calculations and the redundant result assignments in runRemoteJob.
Preserve the existing upload behavior and error handling while ensuring the
returned result uses the supplied values.
In `@internal/jobs/clone_bundle_test.go`:
- Around line 30-32: Update the test helper around the shared ranges variable to
return a snapshot accessor rather than a pointer to the slice. Have the accessor
lock mu, copy the current ranges contents, unlock, and return the snapshot;
update all callers, including the checks near lines 66 and 202-208, to use the
accessor so every read is synchronized.
In `@internal/jobs/clone_bundle.go`:
- Around line 49-58: Update remoteJobBundleInfo and the bundle materialization
flow to preserve the server’s defaultBranch value and check out that branch
after cloning. Make materializeClone use job.Origin when set, falling back to
the effective reconstructed URL, matching runCloneLayered’s origin behavior;
retain the existing fallback when Origin is empty.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: ff11380d-23f8-4210-a19f-92a08e1bb4df
📒 Files selected for processing (7)
internal/git/bundle.gointernal/http/jobs2.gointernal/http/router.gointernal/jobs/clone.gointernal/jobs/clone_bundle.gointernal/jobs/clone_bundle_test.goweb/index.html
Add context/timeouts to git commands, prevent git args injection with '--', and improve git output handling. Introduce concurrency semaphore, per-job timeout, cancellation, and max bundle size checks for remote jobs. Add per-IP v2 rate limiter and size middleware in router. Improve Openbin upload to accept precomputed size/hash, set mime and Content-Length, and use a timed HTTP client with optional API key. Fix range-download edge cases, ensure default-branch checkout and origin handling, remove cached bundles after materialization, and update tests and minor web copy typo.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/http/jobs2.go (1)
138-224: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winCancelling a job does not stop the running clone or upload.
runRemoteJobcreatesctxwith only a 30-minute timeout (Line 163-164); nothing ties it to job cancellation.jobCancelled(job.ID)is checked at Line 193 and Line 218, but both checks run only after the expensive step (git.CreateBundle,openbinUpload) has already completed.DeleteRemoteJobHandler(Line 141-150) only flipsStatustorjCancelled— it never signals the running goroutine to stop.As a result, DELETE
/v2/jobs/:iddoes not free the worker's concurrency slot early, does not stop the mirror clone, and does not stop the Openbin upload. It only prevents publishing the finalreadyresult. This defeats the purpose of exposing cancellation on a resource-bounded (3 concurrent workers) endpoint: a caller cannot actually abort a stuck or unwanted job to make room for another.Store the job's
context.CancelFuncwhereDeleteRemoteJobHandlercan reach it (for example, a smallmap[string]context.CancelFuncguarded by a mutex, or a field alongside the job inremoteJobs), and call it from the delete handler.exec.CommandContext-backed git commands inbundle.gowill then stop promptly.🛡️ Proposed direction
+var ( + remoteJobCancelsMu sync.Mutex + remoteJobCancels = map[string]context.CancelFunc{} +) + func runRemoteJob(job *remoteJob) { defer func() { <-remoteJobSlots }() ctx, cancel := context.WithTimeout(context.Background(), remoteJobTimeout) defer cancel() + remoteJobCancelsMu.Lock() + remoteJobCancels[job.ID] = cancel + remoteJobCancelsMu.Unlock() + defer func() { + remoteJobCancelsMu.Lock() + delete(remoteJobCancels, job.ID) + remoteJobCancelsMu.Unlock() + }()func DeleteRemoteJobHandler(c *gin.Context) { id := c.Param("id") if job, ok := remoteJobs.Get(id); ok && job.Status != rjCancelled { next := *job next.Status = rjCancelled next.Progress = "Cancelado" remoteJobs.Set(id, &next) + remoteJobCancelsMu.Lock() + if cancel, ok := remoteJobCancels[id]; ok { + cancel() + } + remoteJobCancelsMu.Unlock() } c.Status(http.StatusNoContent) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/http/jobs2.go` around lines 138 - 224, Connect job cancellation to the running operation: store each runRemoteJob context.CancelFunc in a concurrency-safe registry accessible by DeleteRemoteJobHandler, invoke and remove it when the job is deleted, and clean it up when runRemoteJob exits. Keep the existing status update and timeout behavior, while ensuring the context passed to git.CreateBundle and the upload path is cancelled promptly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/git/bundle.go`:
- Around line 22-67: Update CreateBundle after the shallow-history completion
logic and before bundle create to explicitly fetch tags from origin using git
fetch --tags origin. Keep the existing history deepening and bundle creation
flow unchanged, ensuring tags for newly reached commits are included in the
generated bundle.
---
Outside diff comments:
In `@internal/http/jobs2.go`:
- Around line 138-224: Connect job cancellation to the running operation: store
each runRemoteJob context.CancelFunc in a concurrency-safe registry accessible
by DeleteRemoteJobHandler, invoke and remove it when the job is deleted, and
clean it up when runRemoteJob exits. Keep the existing status update and timeout
behavior, while ensuring the context passed to git.CreateBundle and the upload
path is cancelled promptly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 377c6614-9457-4967-9e3d-dae152bbe40c
📒 Files selected for processing (7)
internal/git/bundle.gointernal/http/jobs2.gointernal/http/router.gointernal/jobs/clone.gointernal/jobs/clone_bundle.gointernal/jobs/clone_bundle_test.goweb/index.html
🚧 Files skipped from review as they are similar to previous changes (3)
- internal/jobs/clone.go
- web/index.html
- internal/jobs/clone_bundle.go
Add URL validation in internal/git to avoid git option injection and restrict clone hosts/paths; fetch tags after shallow mirror clones so tags are included in bundles. Add cancelation support for remote jobs: store per-job CancelFunc in a sync.Map, let DELETE cancel running workers, and avoid overwriting a "Canceled" state when context.Canceled occurs. Propagate context through openbinUpload/openbinPost/openbinPut so uploads can be canceled. Remove the right sidebar from web/index.html. Minor import additions (net/url, sync).
Replace the boolean validCloneURL with safeCloneURL which validates and returns a normalized URL string or an error. CreateBundle now calls safeCloneURL and passes the sanitized URL to git clone, ensuring the raw user input is never used as a command argument. Preserves the same hosts/path checks and rejection of credentials/non-HTTPS, and clarifies comments about import-cycle constraints. Improves error reporting when the repository URL is invalid. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
gitgost-anonymous
previously approved these changes
Aug 1, 2026
Strip all Spanish-language inline comments from internal/cli (cli.go, proc_unix.go, proc_windows.go, rewrite.go, rewrite_test.go) and internal/git (bundle.go, push.go, receive.go). Replace safeCloneURL's url.Parse validation with a single regex pattern (repoURLPattern) that validates and captures allowed hosts (github.com, gitlab.com, codeberg.org), optional port, and exactly two path segments (owner/repo), then reconstructs the URL from matched groups to ensure user input
gitgost-anonymous
previously approved these changes
Aug 1, 2026
…ive URL safety tests Extract git clone into dedicated gitClone function that validates URL at the exact point it becomes a git argument, preventing option injection across all call paths. Add explicit regex barrier in safeCloneURL with early MatchString check before FindStringSubmatch to satisfy static analysis. Remove Spanish comments from bundle.go, handlers.go. Add 16 test cases covering host validation, path traversal, option injection, userinfo/fragment rejection, port
gitgost-anonymous
previously approved these changes
Aug 1, 2026
Strip all Spanish-language inline comments from handlers.go (isPanicMode, isGlobalBurstAlertActive, recordGlobalBurst, notifyAdminGlobalBurst, checkRateLimit, notifyAdminRateLimit, PanicHandler, ServiceStatusHandler, RollbackBurstHandler, InitDatabase, RecordPR, StatsHandler, RecentPRsHandler, CreateAnonymousIssueHandler, GitLabIssueNotesProxyHandler, GitLabCommitCountHandler, GitLabAvatarHandler, GitLabCommitsHandler, GitLabCommitDetailHandler, GitHubDiscussionsProxyHandler, GitHubDiscussionDetailPro
gitgost-anonymous
approved these changes
Aug 1, 2026
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
3 participants
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.
Introduce Phase 2 remote-job flow: server-side bundling and Openbin upload, plus client support to use those bundles with byte-range resume.
Key changes:
This enables creating bundles server-side, serving them via CDN (Openbin/Filebase), and robust client downloads with resume and integrity checks.
Summary by CodeRabbit