Add git-gost CLI, jobs queue & client build - #126
Conversation
Introduce a full client CLI and local job queue: adds cmd/gost main and internal/cli (install, clone, jobs, watch, pause/resume/cancel, rewrite rules, platform process handling). Adds internal/jobs (store backed by modernc.org/sqlite, run/retry logic, layered clone with resume, unit tests). Updates internal HTTP handlers to preserve protocol/query params and increase timeouts. Adds web UI content for the Git extension and a GitHub Actions workflow to build and publish platform binaries with SHA/Sigstore attestations. Also updates go.mod/go.sum for new deps and test utilities.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
📝 WalkthroughWalkthroughThe change adds a cross-platform Changesgit-gost client workflow
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant cli.Run
participant Store
participant jobs.Run
participant Git
User->>cli.Run: Request background clone
cli.Run->>Store: Create queued job
cli.Run->>jobs.Run: Start job ID
jobs.Run->>Git: Execute resumable clone
Git-->>jobs.Run: Progress and result
jobs.Run->>Store: Persist job state
User->>cli.Run: Monitor job
cli.Run->>Store: Read progress and state
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (9)
internal/jobs/store.go-38-43 (1)
38-43: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winRestrict the data directory permissions to the owner.
The queue database can contain remote URLs with embedded credentials, because
Job.URLandJob.Originstore whatever URL the user passes. Mode0o755makes the directory readable by every local user. Use0o700for per-user application data.🔒 Proposed fix
if dir := filepath.Dir(path); dir != "" { - if err := os.MkdirAll(dir, 0o755); err != nil { + if err := os.MkdirAll(dir, 0o700); err != nil { return nil, fmt.Errorf("crear directorio de datos: %w", 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/jobs/store.go` around lines 38 - 43, Update the MkdirAll call in the data-directory setup to use mode 0o700 instead of 0o755, keeping the existing directory creation and error handling unchanged.internal/jobs/clone_test.go-14-24 (1)
14-24: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winPin the git identity in
buildRepo.
git commitrequiresuser.nameanduser.email.buildReporelies on the ambient git configuration, so the test fails withAuthor identity unknownon any machine or CI container without a global git config.runGitdiscards stderr, so the failure appears only ascommit 0: exit status 128.Set the identity explicitly on the test repository.
💚 Proposed fix
if err := runGit(src, "checkout", "-q", "-b", "main"); err != nil { t.Fatalf("crear main: %v", err) } + if err := runGit(src, "config", "user.email", "test@gitgost.local"); err != nil { + t.Fatalf("config user.email: %v", err) + } + if err := runGit(src, "config", "user.name", "gitgost test"); err != nil { + t.Fatalf("config user.name: %v", err) + } for i := 0; i < n; i++ {🤖 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_test.go` around lines 14 - 24, Update buildRepo to configure a repository-local Git user.name and user.email before its commit loop, using runGit against src and failing through the existing test error pattern if configuration fails. Keep the identity setup scoped to this test repository and ensure it occurs after initialization and before the first git commit.internal/jobs/run.go-44-47 (1)
44-47: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReject an empty
job.CWDbefore running fetch, pull, or push.
cmd.Diris empty whenjob.CWDis empty, sogitruns in the working directory of the calling process. The CLI launches the runner as a detached background process, so that directory is not necessarily the user's repository. The operation then targets the wrong repository or fails with a confusing message.runClonealready guards its equivalent field at lines 23-25 ofinternal/jobs/clone.go.🛡️ Proposed fix
func runGitOperation(s *Store, job *Job) error { + if job.CWD == "" { + return fmt.Errorf("job de %s sin directorio de trabajo", job.Operation) + } args := append([]string{job.Operation}, job.Args...) return execGitWithRetry(s, job, job.CWD, args...) }🤖 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/run.go` around lines 44 - 47, Update runGitOperation to reject jobs with an empty job.CWD before constructing arguments or calling execGitWithRetry; return an appropriate error consistent with the existing runClone validation, while preserving the current fetch, pull, and push execution path for valid working directories.internal/jobs/store.go-43-44 (1)
43-44: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winAvoid treating plain paths as URI DSNs.
modernc.org/sqlitev1.55.0 parses the DSN as afile:URI and extracts query parameters from the first?. A path value containing?,#, or%can split the DSN or be decoded as URI syntax; build the DSN with URL/query encoding, or use a non-URI form if the driver accepts it.🤖 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/store.go` around lines 43 - 44, Update the DSN construction before sql.Open in the store initialization to safely encode the database path when it contains ?, #, or %. Preserve the existing WAL and busy-timeout pragmas, and use the driver-supported URL/query encoding approach rather than concatenating the raw path into the file: URI.internal/jobs/clone.go-63-64 (1)
63-64: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winMake blob-filtered fetches persist through
origin.After the configured
remote origin, these fetches usejob.URLdirectly. Usefetch origin ...and persistremote.origin.promisor=trueplusremote.origin.partialclonefilter=blob:nonebefore the first fetch so the subsequent lazy checkout can fetch missing blobs, instead of relying on the tests’ localfile://source.🤖 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.go` around lines 63 - 64, Update the clone setup before the first fetch in the relevant job flow to configure the existing origin remote with promisor settings: set remote.origin.promisor=true and remote.origin.partialclonefilter=blob:none. Change the fetch invocation in the shown execGitWithRetry call to use origin instead of job.URL, preserving the existing depth, blob filter, and refspec arguments so lazy checkout can retrieve missing blobs..github/workflows/release.yml-142-143 (1)
142-143: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winDisable credential persistence in the client checkout.
actions/checkoutwrites the job token into.git/configby default. The build job only needs source code. Setpersist-credentials: falseso the token cannot reach build output or a later step.🛡️ Proposed fix
- name: Checkout uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd + with: + persist-credentials: false🤖 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 @.github/workflows/release.yml around lines 142 - 143, Update the actions/checkout step in the release workflow to set persist-credentials to false, ensuring the checkout retrieves source code without storing the job token in .git/config.Source: Linters/SAST tools
web/index.html-1648-1648 (1)
1648-1648: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winUse HTTPS for the Software Freedom Conservancy link and add
rel.The new link uses
http://sfconservancy.org/. The sibling GitHub link on the same line useshttpswithtarget="_blank" rel="noopener". Match it.🔒️ Proposed fix
-Git is a trademark of the <a href="http://sfconservancy.org/" style="color:var(--accent);text-decoration:none;">Software Freedom Conservancy</a>. +Git is a trademark of the <a href="https://sfconservancy.org/" target="_blank" rel="noopener" style="color:var(--accent);text-decoration:none;">Software Freedom Conservancy</a>.🤖 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 `@web/index.html` at line 1648, Update the Software Freedom Conservancy anchor in the surrounding footer text to use the HTTPS URL and add the same appropriate rel attribute as the neighboring external GitHub link, while preserving the existing styling and text.web/index.html-3682-3685 (1)
3682-3685: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winThe pause/resume/cancel claim is not true on Windows.
The card states that the user can pause, resume and cancel any job at any time.
internal/cli/proc_windows.goreturns an error for all three actions while a job runs. The same page offers the Windows binary at Line 3639. Qualify the claim, or state the current platform limit.📝 Proposed wording
- <p style="margin:0;font-size:0.78rem;color:var(--fg-muted);">Network failures are detected and retried with exponential backoff. Pause, resume and cancel any job at any time.</p> + <p style="margin:0;font-size:0.78rem;color:var(--fg-muted);">Network failures are detected and retried with exponential backoff. Pause, resume and cancel any job at any time on macOS and Linux; on Windows, control of a running job is not available yet.</p>🤖 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 `@web/index.html` around lines 3682 - 3685, Update the “Retrying state with backoff” card text to avoid claiming pause, resume, and cancel support for every job; explicitly qualify these controls by platform or state that they are unavailable for running jobs on Windows. Keep the existing retry and exponential-backoff description unchanged.internal/cli/proc_windows.go-25-28 (1)
25-28: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winAdd a console-detach flag for this background process.
CREATE_NEW_PROCESS_GROUPisolates console control signals, but the child remains attached to the parent console unless another creation flag changes console behavior. To avoid a visible console window while keeping a console for a console subsystem, addCREATE_NO_WINDOW; useDETACHED_PROCESSonly if the child must not inherit the parent console at all.🤖 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/cli/proc_windows.go` around lines 25 - 28, Add syscall.CREATE_NO_WINDOW to the CreationFlags in the SysProcAttr used by the command setup, alongside CREATE_NEW_PROCESS_GROUP, so the background process does not display a console window while retaining console-subsystem behavior. Use DETACHED_PROCESS only if this flow explicitly requires complete detachment from the parent console.
🧹 Nitpick comments (10)
internal/jobs/store.go (1)
141-146: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConstrain
columnto a fixed set to satisfy the SQL-injection linters.Static analysis flags the
fmt.Sprintfinterpolation here. The current code is not exploitable, becauseSetState,SetProgress, andSetErrorare the only callers and each passes a literal column name. The finding is still worth closing: it silences the recurring lint error and prevents a future caller from passing dynamic input.♻️ Proposed refactor with an explicit column allowlist
+var touchableColumns = map[string]string{ + "state": `UPDATE jobs SET state = ?, updated_at = ? WHERE id = ?`, + "progress": `UPDATE jobs SET progress = ?, updated_at = ? WHERE id = ?`, + "error": `UPDATE jobs SET error = ?, updated_at = ? WHERE id = ?`, +} + func (s *Store) touch(id int64, column, value string) error { - _, err := s.db.Exec( - fmt.Sprintf(`UPDATE jobs SET %s = ?, updated_at = ? WHERE id = ?`, column), - value, time.Now().UTC().Format(time.RFC3339Nano), id) - return err + query, ok := touchableColumns[column] + if !ok { + return fmt.Errorf("columna no permitida: %s", column) + } + _, err := s.db.Exec(query, value, time.Now().UTC().Format(time.RFC3339Nano), id) + return 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/jobs/store.go` around lines 141 - 146, Constrain the column interpolation in Store.touch to an explicit allowlist containing only the columns used by SetState, SetProgress, and SetError; reject any other value before executing the update, while preserving the existing timestamp and job-ID behavior.Source: Linters/SAST tools
internal/jobs/clone_test.go (1)
83-86: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReproduce the real checkpoint with the same fetch flags.
runClonecreates its first block withfetch --depth=<chunk> --filter=blob:noneat lines 63-64 ofinternal/jobs/clone.go. The setup here omits--filter=blob:none, so the checkpoint that the test resumes from holds full blobs. The test therefore does not exercise the resume path that production creates, which is the path where the partial-clone configuration matters.💚 Proposed fix
refspec := "+refs/heads/*:refs/remotes/origin/*" - if err := runGit(dest, "fetch", "--depth=100", "file://"+src, refspec); err != nil { + if err := runGit(dest, "fetch", "--depth=100", "--filter=blob:none", "file://"+src, refspec); err != nil { t.Fatalf("checkpoint inicial: %v", 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/jobs/clone_test.go` around lines 83 - 86, Update the checkpoint setup in the test around runGit to include the same --filter=blob:none flag used by runClone’s initial fetch, while preserving the existing depth, source, and refspec arguments so resume testing starts from a production-equivalent partial clone.internal/jobs/retry.go (2)
117-127: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winSeparate the empty-line check from the nil-progress check.
Line 119 skips the iteration when
progress == nil, solastLineis never assigned in that case. The error returned at line 138 then omits the git message, andisRetryablecannot classify the failure. All current callers pass a non-nil callback fromprogressFn, so the defect is latent. AssignlastLinefirst, then call the callback only if it exists.The truncation at line 123 slices by bytes. A localized git message can be cut in the middle of a UTF-8 rune, and the invalid bytes are then persisted in
progress. Slice by runes instead.♻️ Proposed refactor
for scanner.Scan() { line := strings.TrimSpace(scanner.Text()) - if line == "" || progress == nil { + if line == "" { continue } - if len(line) > 160 { - line = line[len(line)-160:] + if r := []rune(line); len(r) > 160 { + line = string(r[len(r)-160:]) } lastLine = line - progress(line) + if progress != nil { + progress(line) + } }🤖 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/retry.go` around lines 117 - 127, Update the scanner loop in the retry handling flow to always assign trimmed, rune-safe truncated text to lastLine regardless of whether progress is nil, then invoke progress only when the callback exists; keep empty lines skipped and replace byte-based truncation with rune-based truncation capped at 160 characters.
77-97: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the duplicated retry loop. Both functions implement the identical attempt loop, exponential backoff,
StateRetryingtransition, and progress message. Only the executed operation differs. The shared root cause is one retry policy written twice, so a change toretryMax, the backoff growth, or the progress text must be applied in two places.Extract a helper such as
withRetry(s *Store, jobID int64, op func() error) errorand let both call sites supply the operation.
internal/jobs/retry.go#L77-L97: reduceexecGitWithRetryto awithRetrycall that wrapsrunGitCapture.internal/jobs/clone.go#L166-L189: reducegitOutputWithRetryto awithRetrycall that wrapsgitOutputand capturesoutin the closure.🤖 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/retry.go` around lines 77 - 97, The retry policy is duplicated across both operations. In internal/jobs/retry.go lines 77-97, add a shared withRetry helper and reduce execGitWithRetry to supplying runGitCapture as its operation; in internal/jobs/clone.go lines 166-189, reduce gitOutputWithRetry to the same helper while capturing out from gitOutput in the closure. Preserve the existing retry limits, exponential backoff, StateRetrying transition, progress message, and error handling in withRetry.internal/jobs/clone.go (2)
123-126: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winDo not treat a
rev-parsefailure as "history complete".
repoShallowreturnsfalseboth when the repository is complete and when thegit rev-parsecall fails.runCloneuses that value as the loop condition at line 73 and as the--unshallowguard at line 94. Ifrev-parsefails transiently,runCloneexits the deepen loop, skips--unshallow, and returnsnil.Runthen marks the jobcompletedwhile the repository still has partial history, and the user receives no error.Return the error and let
runClonepropagate it, so a failed probe retries or fails the job instead of reporting success.🤖 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.go` around lines 123 - 126, Update repoShallow to propagate the gitOutput error instead of converting rev-parse failures into false; adjust its callers in runClone to handle the returned error and propagate it, while preserving the shallow-state result for successful probes so failed checks cannot report a completed clone.
29-31: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake
runGitfailures diagnosable.
runGitcallscmd.Run()and discards both stdout and stderr. Each wrapped error therefore reports onlyexit status 1. The user seescrear rama local main: exit status 1in the joberrorcolumn with no cause. The truncated message is then persisted byRunand displayed bygit gost list.
gitOutputalready attaches the git message to the error. Use it for these call sites, or capture stderr insiderunGit.♻️ Proposed refactor
// runGit ejecuta git sin capturar salida. func runGit(dir string, args ...string) error { - cmd := exec.Command("git", args...) - cmd.Dir = dir - return cmd.Run() + _, err := gitOutput(dir, args...) + return err }This also improves
isRetryableclassification for the errors that reach it from these paths.Also applies to: 41-43, 103-108, 192-196
🤖 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.go` around lines 29 - 31, Update the git operation call sites in the clone job, including the shown repository initialization and the referenced branch-related paths, to use gitOutput instead of runGit so command output is preserved in returned errors. Keep the existing wrapped error context and behavior unchanged while ensuring failures include Git’s diagnostic message for persistence and retry classification.internal/cli/cli.go (1)
431-469: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
watchnever returns for a paused job.The loop exits only on
StateCompletedandStateFailed. A paused job keeps the loop polling every 500 ms with no output. Print the paused state once and return, or document that the user must interrupt the command.🤖 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/cli/cli.go` around lines 431 - 469, Update cmdWatch so a job in the paused state is handled explicitly: print the paused status once and return instead of continuing the 500 ms polling loop. Preserve the existing progress output and completed/failed handling.internal/cli/proc_windows.go (1)
46-50: 🩺 Stability & Availability | 🔵 Trivial | 🏗️ Heavy liftWindows can support cancel, even without POSIX signals.
signalJobrejects every request on Windows. Pause and resume need job objects, so the current limit is reasonable for them. Cancel does not:os.FindProcessplusProcess.Killterminates the process withouttaskkill. Implement cancel so thatgit gost cancelworks on Windows, and keep the error for pause and resume only.♻️ Proposed direction
-func signalJob(pid int, sig syscall.Signal) error { +func signalJob(pid int, sig syscall.Signal) error { + if sig == sigTerm { + if pid <= 0 { + return fmt.Errorf("el job no tiene proceso en background activo") + } + p, err := os.FindProcess(pid) + if err != nil { + return err + } + return p.Kill() + } return fmt.Errorf("pause/resume/cancel en ejecución no está soportado en Windows en esta fase") }Note that
sigStop,sigContandsigTermare allsyscall.Signal(0)on Windows, so they are not distinguishable. Give them distinct sentinel values before you branch on them.🤖 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/cli/proc_windows.go` around lines 46 - 50, Update signalJob to distinguish cancel from pause/resume using distinct sentinel values for sigStop, sigCont, and sigTerm on Windows, then handle the cancel sentinel by locating the process with os.FindProcess and terminating it via Process.Kill. Preserve the unsupported-operation error for pause and resume, and update the signal definitions so they remain distinguishable on Windows.internal/cli/proc_unix.go (1)
28-38: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider a log file instead of
/dev/nullfor the child.The detached job writes stdout and stderr to
os.DevNull. Diagnostics for a failed background job then depend only on theErrorfield in the store. A rotating log file underdataDir(), for example~/.gitgost/logs/job-<id>.log, makes support and debugging practical.🤖 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/cli/proc_unix.go` around lines 28 - 38, The detached child process currently discards output through os.DevNull; update the command setup around cmd.Stdin, cmd.Stdout, and cmd.Stderr to create a per-job log file under dataDir(), such as logs/job-<id>.log, and route the child’s stdout and stderr to it while preserving detached execution. Ensure the log directory and file are created using the existing job identifier and close the file appropriately.internal/cli/rewrite.go (1)
40-47: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winReport unsupported nested GitLab groups explicitly.
The server routes only expose
/v1/<prefix>/owner/repo, buthttps://gitlab.com/group/subgroup/repo.gitis a valid GitLab URL.RewriteURLcurrently reports it asURL de repositorio inválida. Return a limitation-specific error for nested paths before falling back to the invalid error.🤖 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/cli/rewrite.go` around lines 40 - 47, Update RewriteURL’s path validation around splitPath so paths with more than two segments return a limitation-specific error indicating nested GitLab groups are unsupported; retain the existing invalid-repository error for paths with fewer than two segments or invalid owner/repo segments.
🤖 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 @.github/workflows/release.yml:
- Around line 150-160: Update the release workflow’s Get commit info step and
the build, sha, and checksum steps to pass github.ref, github.ref_name, matrix
values, and steps.*.outputs.* through each step’s env block, then reference
those environment variables inside run scripts instead of interpolating GitHub
expressions directly. Preserve the existing version-selection and output
behavior while ensuring all shell inputs are treated as data.
- Around line 212-218: Consolidate release publishing so only one job invokes
softprops/action-gh-release for each tag. Update the build-and-attest and
build-client matrix flow to collect or pass all generated binaries and checksums
to a single release job, ensuring that job publishes every artifact without
concurrent release updates; alternatively, add a tag-based concurrency group
that serializes the existing release actions.
In `@internal/cli/cli.go`:
- Around line 472-508: Update cmdPause and cmdResume to validate that the stored
j.PID still belongs to the target job before calling signalJob. Add and persist
a job-process identity marker, such as the child’s start time or token, and
compare it with the current process identity after syscall.Kill(pid, 0) confirms
liveness. If the identity does not match or the process is absent, do not signal
the process group and handle the stale state safely.
- Around line 356-367: Update launchBackground to persist the intended
pre-launch state before calling startBackground, then remove the post-launch
SetState write so only SetPID persists after the child starts. Ensure cmdResume
and other callers do not overwrite running, completed, or failed states reported
by jobs.Run.
- Around line 328-338: Update parseFlags to inspect only leading CLI flags and
stop at the first non-flag argument, preserving that argument and all subsequent
Git arguments—including -f—for downstream handling; retain foreground parsing
only before the command. Remove the always-nil err return or update cmdClone and
cmdGitJob to handle the revised signature consistently.
In `@internal/cli/proc_windows.go`:
- Around line 46-50: Implement Windows cancellation in signalJob using
os.FindProcess and Process.Kill, with distinct sigStop, sigCont, and sigTerm
sentinel values for action branching. In internal/cli/cli.go lines 517-520,
propagate signalJob errors from cmdCancel and set StateFailed only after the
process stops. In web/index.html lines 3682-3685, document that pause and resume
are unavailable on Windows until job objects are supported.
In `@internal/http/handlers.go`:
- Around line 37-40: Update the upstream GET and POST request construction in
the relevant handlers to use http.NewRequestWithContext with c.Request.Context()
instead of http.NewRequest, preserving the existing request methods, URLs, and
bodies so client disconnects cancel upstream work promptly.
- Around line 663-676: The UploadPackDiscoveryHandler discovery proxy must
forward the client’s Git-Protocol header to the remote request, since query
parameters do not select protocol v2. Update the remote discovery request
construction around remoteURL to propagate the incoming Git-Protocol value, and
extend the discovery proxy test to verify this header is forwarded.
In `@internal/jobs/retry.go`:
- Around line 34-36: Update isRetryable’s noRetry status matching for 401, 403,
404, 502, 503, and 504 to require surrounding git or curl HTTP-error text rather
than matching bare numeric substrings. Preserve recognition of existing forms
such as “HTTP 502” and “returned error: 504”, while preventing counters, ports,
URLs, or other incidental numbers from being classified as permanent failures.
- Around line 102-104: Force a stable English locale for both git command
builders by setting cmd.Env to the existing process environment plus LC_ALL=C
and LANG=C. Apply this in runGitCapture in internal/jobs/retry.go at lines
102-104 before cmd.Start(), and in gitOutput in internal/jobs/clone.go at lines
153-156 before cmd.CombinedOutput().
- Around line 129-133: Update the scanner.Err() failure path to call cmd.Wait()
after cmd.Process.Kill() and before returning the scanner error, ensuring the
child process is reaped and StderrPipe resources are released.
In `@internal/jobs/run.go`:
- Around line 19-24: Replace the separate state validation and SetState calls in
Run with an atomic claim through a new Store.Claim method. Implement Claim in
internal/jobs/store.go using a conditional UPDATE that excludes Running,
Completed, and Failed states, then verify RowsAffected; have Run return the
existing terminal-state error when the claim is unsuccessful and propagate
database errors.
---
Minor comments:
In @.github/workflows/release.yml:
- Around line 142-143: Update the actions/checkout step in the release workflow
to set persist-credentials to false, ensuring the checkout retrieves source code
without storing the job token in .git/config.
In `@internal/cli/proc_windows.go`:
- Around line 25-28: Add syscall.CREATE_NO_WINDOW to the CreationFlags in the
SysProcAttr used by the command setup, alongside CREATE_NEW_PROCESS_GROUP, so
the background process does not display a console window while retaining
console-subsystem behavior. Use DETACHED_PROCESS only if this flow explicitly
requires complete detachment from the parent console.
In `@internal/jobs/clone_test.go`:
- Around line 14-24: Update buildRepo to configure a repository-local Git
user.name and user.email before its commit loop, using runGit against src and
failing through the existing test error pattern if configuration fails. Keep the
identity setup scoped to this test repository and ensure it occurs after
initialization and before the first git commit.
In `@internal/jobs/clone.go`:
- Around line 63-64: Update the clone setup before the first fetch in the
relevant job flow to configure the existing origin remote with promisor
settings: set remote.origin.promisor=true and
remote.origin.partialclonefilter=blob:none. Change the fetch invocation in the
shown execGitWithRetry call to use origin instead of job.URL, preserving the
existing depth, blob filter, and refspec arguments so lazy checkout can retrieve
missing blobs.
In `@internal/jobs/run.go`:
- Around line 44-47: Update runGitOperation to reject jobs with an empty job.CWD
before constructing arguments or calling execGitWithRetry; return an appropriate
error consistent with the existing runClone validation, while preserving the
current fetch, pull, and push execution path for valid working directories.
In `@internal/jobs/store.go`:
- Around line 38-43: Update the MkdirAll call in the data-directory setup to use
mode 0o700 instead of 0o755, keeping the existing directory creation and error
handling unchanged.
- Around line 43-44: Update the DSN construction before sql.Open in the store
initialization to safely encode the database path when it contains ?, #, or %.
Preserve the existing WAL and busy-timeout pragmas, and use the driver-supported
URL/query encoding approach rather than concatenating the raw path into the
file: URI.
In `@web/index.html`:
- Line 1648: Update the Software Freedom Conservancy anchor in the surrounding
footer text to use the HTTPS URL and add the same appropriate rel attribute as
the neighboring external GitHub link, while preserving the existing styling and
text.
- Around line 3682-3685: Update the “Retrying state with backoff” card text to
avoid claiming pause, resume, and cancel support for every job; explicitly
qualify these controls by platform or state that they are unavailable for
running jobs on Windows. Keep the existing retry and exponential-backoff
description unchanged.
---
Nitpick comments:
In `@internal/cli/cli.go`:
- Around line 431-469: Update cmdWatch so a job in the paused state is handled
explicitly: print the paused status once and return instead of continuing the
500 ms polling loop. Preserve the existing progress output and completed/failed
handling.
In `@internal/cli/proc_unix.go`:
- Around line 28-38: The detached child process currently discards output
through os.DevNull; update the command setup around cmd.Stdin, cmd.Stdout, and
cmd.Stderr to create a per-job log file under dataDir(), such as
logs/job-<id>.log, and route the child’s stdout and stderr to it while
preserving detached execution. Ensure the log directory and file are created
using the existing job identifier and close the file appropriately.
In `@internal/cli/proc_windows.go`:
- Around line 46-50: Update signalJob to distinguish cancel from pause/resume
using distinct sentinel values for sigStop, sigCont, and sigTerm on Windows,
then handle the cancel sentinel by locating the process with os.FindProcess and
terminating it via Process.Kill. Preserve the unsupported-operation error for
pause and resume, and update the signal definitions so they remain
distinguishable on Windows.
In `@internal/cli/rewrite.go`:
- Around line 40-47: Update RewriteURL’s path validation around splitPath so
paths with more than two segments return a limitation-specific error indicating
nested GitLab groups are unsupported; retain the existing invalid-repository
error for paths with fewer than two segments or invalid owner/repo segments.
In `@internal/jobs/clone_test.go`:
- Around line 83-86: Update the checkpoint setup in the test around runGit to
include the same --filter=blob:none flag used by runClone’s initial fetch, while
preserving the existing depth, source, and refspec arguments so resume testing
starts from a production-equivalent partial clone.
In `@internal/jobs/clone.go`:
- Around line 123-126: Update repoShallow to propagate the gitOutput error
instead of converting rev-parse failures into false; adjust its callers in
runClone to handle the returned error and propagate it, while preserving the
shallow-state result for successful probes so failed checks cannot report a
completed clone.
- Around line 29-31: Update the git operation call sites in the clone job,
including the shown repository initialization and the referenced branch-related
paths, to use gitOutput instead of runGit so command output is preserved in
returned errors. Keep the existing wrapped error context and behavior unchanged
while ensuring failures include Git’s diagnostic message for persistence and
retry classification.
In `@internal/jobs/retry.go`:
- Around line 117-127: Update the scanner loop in the retry handling flow to
always assign trimmed, rune-safe truncated text to lastLine regardless of
whether progress is nil, then invoke progress only when the callback exists;
keep empty lines skipped and replace byte-based truncation with rune-based
truncation capped at 160 characters.
- Around line 77-97: The retry policy is duplicated across both operations. In
internal/jobs/retry.go lines 77-97, add a shared withRetry helper and reduce
execGitWithRetry to supplying runGitCapture as its operation; in
internal/jobs/clone.go lines 166-189, reduce gitOutputWithRetry to the same
helper while capturing out from gitOutput in the closure. Preserve the existing
retry limits, exponential backoff, StateRetrying transition, progress message,
and error handling in withRetry.
In `@internal/jobs/store.go`:
- Around line 141-146: Constrain the column interpolation in Store.touch to an
explicit allowlist containing only the columns used by SetState, SetProgress,
and SetError; reject any other value before executing the update, while
preserving the existing timestamp and job-ID behavior.
🪄 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: a2c1b4c4-4286-40fe-9da2-32f312658c08
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (18)
.github/workflows/release.ymlcmd/gost/main.gogo.modinternal/cli/cli.gointernal/cli/proc_unix.gointernal/cli/proc_windows.gointernal/cli/rewrite.gointernal/cli/rewrite_test.gointernal/http/handlers.gointernal/jobs/clone.gointernal/jobs/clone_test.gointernal/jobs/jobs.gointernal/jobs/retry.gointernal/jobs/retry_test.gointernal/jobs/run.gointernal/jobs/store.gointernal/jobs/store_test.goweb/index.html
| - name: Get commit info | ||
| id: info | ||
| run: | | ||
| short=$(git rev-parse --short HEAD) | ||
| echo "short=$short" >> "$GITHUB_OUTPUT" | ||
| echo "built=$(date -u +%Y-%m-%dT%H:%M:%SZ)" >> "$GITHUB_OUTPUT" | ||
| if [[ "${{ github.ref }}" == refs/tags/* ]]; then | ||
| echo "ver=${{ github.ref_name }}" >> "$GITHUB_OUTPUT" | ||
| else | ||
| echo "ver=$short" >> "$GITHUB_OUTPUT" | ||
| fi |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Pass github.ref and github.ref_name through env, not string interpolation.
The run block expands ${{ github.ref }} and ${{ github.ref_name }} directly into bash. A ref name that contains shell metacharacters becomes shell code in the runner. The same value then flows into -ldflags at Line 175. Use environment variables so the shell receives data, not code.
🛡️ Proposed fix
- name: Get commit info
id: info
+ env:
+ GH_REF: ${{ github.ref }}
+ GH_REF_NAME: ${{ github.ref_name }}
run: |
short=$(git rev-parse --short HEAD)
echo "short=$short" >> "$GITHUB_OUTPUT"
echo "built=$(date -u +%Y-%m-%dT%H:%M:%SZ)" >> "$GITHUB_OUTPUT"
- if [[ "${{ github.ref }}" == refs/tags/* ]]; then
- echo "ver=${{ github.ref_name }}" >> "$GITHUB_OUTPUT"
+ if [[ "$GH_REF" == refs/tags/* ]]; then
+ echo "ver=$GH_REF_NAME" >> "$GITHUB_OUTPUT"
else
echo "ver=$short" >> "$GITHUB_OUTPUT"
fiApply the same pattern to the matrix and steps.*.outputs.* expansions in the build, sha and checksum steps.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - name: Get commit info | |
| id: info | |
| run: | | |
| short=$(git rev-parse --short HEAD) | |
| echo "short=$short" >> "$GITHUB_OUTPUT" | |
| echo "built=$(date -u +%Y-%m-%dT%H:%M:%SZ)" >> "$GITHUB_OUTPUT" | |
| if [[ "${{ github.ref }}" == refs/tags/* ]]; then | |
| echo "ver=${{ github.ref_name }}" >> "$GITHUB_OUTPUT" | |
| else | |
| echo "ver=$short" >> "$GITHUB_OUTPUT" | |
| fi | |
| - name: Get commit info | |
| id: info | |
| env: | |
| GH_REF: ${{ github.ref }} | |
| GH_REF_NAME: ${{ github.ref_name }} | |
| run: | | |
| short=$(git rev-parse --short HEAD) | |
| echo "short=$short" >> "$GITHUB_OUTPUT" | |
| echo "built=$(date -u +%Y-%m-%dT%H:%M:%SZ)" >> "$GITHUB_OUTPUT" | |
| if [[ "$GH_REF" == refs/tags/* ]]; then | |
| echo "ver=$GH_REF_NAME" >> "$GITHUB_OUTPUT" | |
| else | |
| echo "ver=$short" >> "$GITHUB_OUTPUT" | |
| fi |
🧰 Tools
🪛 zizmor (1.28.0)
[error] 156-156: code injection via template expansion (template-injection): may expand into attacker-controllable code
(template-injection)
[error] 157-157: code injection via template expansion (template-injection): may expand into attacker-controllable code
(template-injection)
🤖 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 @.github/workflows/release.yml around lines 150 - 160, Update the release
workflow’s Get commit info step and the build, sha, and checksum steps to pass
github.ref, github.ref_name, matrix values, and steps.*.outputs.* through each
step’s env block, then reference those environment variables inside run scripts
instead of interpolating GitHub expressions directly. Preserve the existing
version-selection and output behavior while ensuring all shell inputs are
treated as data.
Source: Linters/SAST tools
| - name: Create/update release on tags | ||
| if: startsWith(github.ref, 'refs/tags/') | ||
| uses: softprops/action-gh-release@153bb8e04406b158c6c84fc1615b65b24149a1fe | ||
| with: | ||
| files: | | ||
| ${{ steps.build.outputs.bin }} | ||
| ${{ steps.build.outputs.bin }}.sha256 |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect the workflow header and any permissions blocks.
sed -n '1,20p' .github/workflows/release.yml
rg -n 'permissions:|id-token|attestations|contents:|concurrency:' .github/workflows/release.ymlRepository: livrasand/gitGost
Length of output: 563
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== workflow outline/line count =="
wc -l .github/workflows/release.yml
echo
echo "== jobs and relevant steps =="
awk '
/^jobs:/ { in_jobs=1; next }
in_jobs && /^[^ ]/ { in_jobs=0; print "---", prev_job, "lines:", start_line, "-", NR-1 }
in_jobs && /^[[:space:]]{2}[a-zA-Z0-9_-]+:/{
start_line=NR; prev_job=$0; gsub(/^ [/[:space:]/, "", prev_job); print "---", prev_job
}
in_jobs && /action-gh-release|attest-build-provenance|build-client|matrix|concurrency|permissions:/{
printf "%d: %s\n", NR, $0
}
in_jobs { prev_job=$0 }
' .github/workflows/release.yml
echo
echo "== action releases/attest usages =="
rg -n 'uses: softprops/action-gh-release|uses:.*/attest-build-provenance|build-client|build-and-attest|matrix:|packages|contents:' .github/workflows/release.yml
echo
echo "== lines 180-225 =="
sed -n '180,225p' .github/workflows/release.yml
echo
echo "== lines 70-95 =="
sed -n '70,95p' .github/workflows/release.ymlRepository: livrasand/gitGost
Length of output: 542
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== workflow line count =="
wc -l .github/workflows/release.yml
echo
echo "== releases/attests/usages =="
rg -n 'uses: softprops/action-gh-release|attest-build-provenance|build-client|build-and-attest|mach-(?:arm64|x64|amd)64|matrix:|packages|contents:' .github/workflows/release.yml || true
echo
echo "== lines 180-218 =="
sed -n '180,218p' .github/workflows/release.yml
echo
echo "== lines 70-110 =="
sed -n '70,110p' .github/workflows/release.yml >/tmp/release_slice.txt
cat -n /tmp/release_slice.txtRepository: livrasand/gitGost
Length of output: 4106
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== lines 110-140 =="
sed -n '110,140p' .github/workflows/release.yml
echo
echo "== full workflow text with line numbers relevant to jobs =="
awk '
NR>=1 && NR<=218 { printf "%d: %s\n", NR, $0 }
' .github/workflows/release.yml >/tmp/release.yml.lined
sed -n '110,135p' /tmp/release.yml.lined
sed -n '1,25p' /tmp/release.yml.linedRepository: livrasand/gitGost
Length of output: 2945
🌐 Web query:
softprops action-gh-release files multiple files append assets documentation
💡 Result:
To upload multiple files using the softprops/action-gh-release GitHub Action, you use the 'files' input [1][2]. This input accepts a newline-delimited list of paths or glob expressions, allowing you to specify multiple files or patterns to include as release assets [1][3][4]. Configuration Example: To upload multiple files, provide them as a multi-line string in your workflow YAML [1][5]: steps: - name: Release uses: softprops/action-gh-release@v3 with: files: | path/to/file1.txt path/to/file2.exe dist/*.tar.gz Key points regarding asset uploading: - Glob Support: The 'files' input supports glob patterns, which are useful for matching multiple files in a directory [1][6][5]. - Working Directory: If your assets are located in a subdirectory, you can use the 'working_directory' input to set the base directory for resolving your 'files' paths [1][2][6]. - Updating Releases: If a release already exists for the given tag, the action will automatically update the release by adding or overwriting the specified assets [1][3]. - Overwriting: By default, the action overwrites existing files with the same name [7][4]. - Preservation of Order: The 'preserve_order' input is available if you need to upload artifacts sequentially, though this does not affect the final display order in the GitHub UI [4]. Documentation regarding appending: The action provides an 'append_body' input (boolean, default is false), which is used for appending text to an existing release body rather than overwriting it [1][4][8]. This setting is distinct from managing release assets (files), which are handled via the 'files' input [1][4].
Citations:
- 1: https://github.com/softprops/action-gh-release
- 2: https://github.com/softprops/action-gh-release/blob/master/README.md
- 3: https://github.com/marketplace/actions/gh-release
- 4: https://github.com/softprops/action-gh-release/blob/master/action.yml
- 5: https://context7.com/softprops/action-gh-release/llms.txt
- 6: https://github.com/softprops/action-gh-release?tab=readme-ov-file
- 7: https://github.com/softprops/action-gh-release/blob/v2/README.md
- 8: https://github.com/softprops/action-gh-release/blob/c062e08bd532815e2082a85e87e3ef29c3e6d191/action.yml
Run release publishing from a single release job.
The workflow has one global permissions block, so id-token and attestations are granted. The remaining hazard is the six concurrent publish actions: one build-and-attest release step plus five build-client matrix release steps. They can create/update the same tag release at the same time and overwrite assets or release metadata. Collect the built artifacts in one job and run softprops/action-gh-release there, or gate the release job with a concurrency group.
🧰 Tools
🪛 zizmor (1.28.0)
[info] 214-214: action functionality is already included by the runner (superfluous-actions): use gh release in a script step
(superfluous-actions)
🤖 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 @.github/workflows/release.yml around lines 212 - 218, Consolidate release
publishing so only one job invokes softprops/action-gh-release for each tag.
Update the build-and-attest and build-client matrix flow to collect or pass all
generated binaries and checksums to a single release job, ensuring that job
publishes every artifact without concurrent release updates; alternatively, add
a tag-based concurrency group that serializes the existing release actions.
| func parseFlags(args []string) (foreground bool, rest []string, err error) { | ||
| for _, a := range args { | ||
| switch a { | ||
| case "-f", "--foreground": | ||
| foreground = true | ||
| default: | ||
| rest = append(rest, a) | ||
| } | ||
| } | ||
| return foreground, rest, nil | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
parseFlags swallows -f meant for Git.
parseFlags scans every argument. git gost push -f therefore runs as a foreground job and drops -f from the Git arguments. The force push does not happen. The same applies to git gost push --force-with-lease? No — only exact -f and --foreground match, but -f is the common Git force flag for push.
Parse only leading flags, and stop at the first non-flag argument. The err return value is also always nil.
🐛 Proposed fix
-func parseFlags(args []string) (foreground bool, rest []string, err error) {
- for _, a := range args {
- switch a {
- case "-f", "--foreground":
- foreground = true
- default:
- rest = append(rest, a)
- }
- }
- return foreground, rest, nil
-}
+func parseFlags(args []string) (foreground bool, rest []string, err error) {
+ i := 0
+ for ; i < len(args); i++ {
+ switch args[i] {
+ case "-f", "--foreground":
+ foreground = true
+ case "--":
+ i++
+ rest = append(rest, args[i:]...)
+ return foreground, rest, nil
+ default:
+ rest = append(rest, args[i:]...)
+ return foreground, rest, nil
+ }
+ }
+ return foreground, rest, nil
+}Update the call sites in cmdClone and cmdGitJob if you keep the always-nil error.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func parseFlags(args []string) (foreground bool, rest []string, err error) { | |
| for _, a := range args { | |
| switch a { | |
| case "-f", "--foreground": | |
| foreground = true | |
| default: | |
| rest = append(rest, a) | |
| } | |
| } | |
| return foreground, rest, nil | |
| } | |
| func parseFlags(args []string) (foreground bool, rest []string, err error) { | |
| i := 0 | |
| for ; i < len(args); i++ { | |
| switch args[i] { | |
| case "-f", "--foreground": | |
| foreground = true | |
| case "--": | |
| i++ | |
| rest = append(rest, args[i:]...) | |
| return foreground, rest, nil | |
| default: | |
| rest = append(rest, args[i:]...) | |
| return foreground, rest, nil | |
| } | |
| } | |
| return foreground, rest, nil | |
| } |
🤖 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/cli/cli.go` around lines 328 - 338, Update parseFlags to inspect
only leading CLI flags and stop at the first non-flag argument, preserving that
argument and all subsequent Git arguments—including -f—for downstream handling;
retain foreground parsing only before the command. Remove the always-nil err
return or update cmdClone and cmdGitJob to handle the revised signature
consistently.
| func launchBackground(s *jobs.Store, id int64) int { | ||
| pid, err := startBackground(id) | ||
| if err != nil { | ||
| _ = s.SetState(id, jobs.StateFailed) | ||
| _ = s.SetError(id, err.Error()) | ||
| fmt.Fprintf(os.Stderr, "git-gost: %v\n", err) | ||
| return 1 | ||
| } | ||
| _ = s.SetPID(id, pid) | ||
| fmt.Printf("Job created.\n\nID: %d\nRunning in background...\n\nUse 'git gost watch %d' to follow progress.\n", id, id) | ||
| return 0 | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm that jobs.Run performs its own state transitions (running/completed/failed).
fd -t f 'run.go' internal/jobs --exec cat -n
rg -n --type=go 'func \(s \*Store\) (SetState|SetPID|SetError)' internal/jobs/store.goRepository: livrasand/gitGost
Length of output: 2428
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== cli.go relevant sections =="
fd -t f 'cli.go' internal --exec sh -c 'echo "--- $1"; rg -n -C 4 "func launchBackground|cmdResume|startBackground|SetPID|SetState\\(j\\.ID, jobs\\.StateQueued\\)" "$1"' sh {}
echo
echo "== jobs store methods =="
fd -t f 'store.go' internal/jobs --exec sh -c 'echo "--- $1"; cat -n "$1" | sed -n "100,180p"' sh {}
echo
echo "== startBackground definitions/usages =="
rg -n --type=go "func startBackground|startBackground\\(" internalRepository: livrasand/gitGost
Length of output: 4520
🏁 Script executed:
#!/bin/bash
set -euo pipefail
for f in internal/cli/proc_unix.go internal/cli/proc_windows.go; do
echo "--- $f"
cat -n "$f"
echo
done
fd -t f 'store.go' internal/jobs --exec sh -c 'echo "--- $1"; cat -n "$1" | sed -n "1,140p"' sh {}
echo "== signalCommand and SetState usages in signal handling =="
fd -t f 'cli.go' internal --exec sh -c 'echo "--- $1"; rg -n -C 3 "func signalCommand|s\\.SetState|jobs\\.StatePa|jobs\\.StateQueued|jobs\\.StateRunning" "$1"' sh {}Repository: livrasand/gitGost
Length of output: 10566
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== command/state transitions around launchBackground =="
fd -t f 'cli.go' internal --exec sh -c 'echo "--- $1"; rg -n -C 5 "launchBackground\\(|cmdRun|StateRunning|StateCompleted|StateFailed" "$1"' sh {}
echo
echo "== job status/read functions =="
rg -n --type=go 'func cmd(Show|List|Watch|Status)|ReadState|state|completed|StateCompleted|Paused|Queued' internal --glob '*.go' | head -n 200Repository: livrasand/gitGost
Length of output: 15059
Write the final background state before launching the child.
jobs.Run transitions running and can set completed/failed; any parent state write after startBackground can overwrite that result. Move the intended state write before Start, and let only SetPID persist after launch, not SetState. This is especially visible in cmdResume, where writing StateQueued after the child has already reported running or completed reverts the job’s state.
🤖 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/cli/cli.go` around lines 356 - 367, Update launchBackground to
persist the intended pre-launch state before calling startBackground, then
remove the post-launch SetState write so only SetPID persists after the child
starts. Ensure cmdResume and other callers do not overwrite running, completed,
or failed states reported by jobs.Run.
| func cmdPause(args []string) int { | ||
| return signalCommand("pause", args, func(j *jobs.Job, s *jobs.Store) (int, error) { | ||
| switch j.State { | ||
| case jobs.StateQueued, jobs.StateRetrying: | ||
| return 0, s.SetState(j.ID, jobs.StatePaused) | ||
| case jobs.StateRunning: | ||
| if err := signalJob(j.PID, sigStop); err != nil { | ||
| return 0, err | ||
| } | ||
| return 0, s.SetState(j.ID, jobs.StatePaused) | ||
| default: | ||
| return 0, fmt.Errorf("el job %d no se puede pausar (estado: %s)", j.ID, j.State) | ||
| } | ||
| }) | ||
| } | ||
|
|
||
| // cmdResume reanuda un job pausado. | ||
| func cmdResume(args []string) int { | ||
| return signalCommand("resume", args, func(j *jobs.Job, s *jobs.Store) (int, error) { | ||
| if j.State != jobs.StatePaused { | ||
| return 0, fmt.Errorf("el job %d no está pausado (estado: %s)", j.ID, j.State) | ||
| } | ||
| if j.PID > 0 { | ||
| if err := signalJob(j.PID, sigCont); err != nil { | ||
| return 0, err | ||
| } | ||
| return 0, s.SetState(j.ID, jobs.StateRunning) | ||
| } | ||
| // Pausado sin proceso (estaba en cola): relanzar en background. | ||
| pid, err := startBackground(j.ID) | ||
| if err != nil { | ||
| return 0, err | ||
| } | ||
| _ = s.SetPID(j.ID, pid) | ||
| return 0, s.SetState(j.ID, jobs.StateQueued) | ||
| }) | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
A stale PID can signal an unrelated process group.
cmdPause and cmdResume read j.PID from the store and call signalJob(j.PID, …), which sends the signal to the whole process group -pid on Unix. If the recorded process already exited and the operating system reused the PID, gitGost stops or continues a process group that belongs to another program. The store state can stay running after a crash, so this path is reachable.
Verify that the recorded process is still the job process before you signal it. Check liveness with syscall.Kill(pid, 0) and store an additional identity marker, for example the process start time or a job token that the child records at startup.
🤖 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/cli/cli.go` around lines 472 - 508, Update cmdPause and cmdResume to
validate that the stored j.PID still belongs to the target job before calling
signalJob. Add and persist a job-process identity marker, such as the child’s
start time or token, and compare it with the current process identity after
syscall.Kill(pid, 0) confirms liveness. If the identity does not match or the
process is absent, do not signal the process group and handle the stale state
safely.
| // Reenviar los query params del cliente (protocol=v2 y otros) además del | ||
| // service: sin protocol=v2 el servidor remoto responde en protocolo v1 y | ||
| // git no puede usar filtros (partial clone) ni la negociación eficiente. | ||
| q := url.Values{} | ||
| q.Set("service", "git-upload-pack") | ||
| for k, vals := range c.Request.URL.Query() { | ||
| if k == "service" { | ||
| continue | ||
| } | ||
| for _, v := range vals { | ||
| q.Add(k, v) | ||
| } | ||
| } | ||
| remoteURL := prov.CloneURL(owner, repo) + "/info/refs?" + q.Encode() |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 6 'UploadPackDiscoveryHandler|Git-Protocol|NewRequest' internal/http/handlers.go
rg -n -C 6 'TestUploadPackDiscoveryHandler_ProxiesGitHub|Git-Protocol' internal/http/handlers_upload_test.goRepository: livrasand/gitGost
Length of output: 10851
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== handlers.go relevant section =="
sed -n '656,705p' internal/http/handlers.go
echo "== test relevant section =="
sed -n '111,180p' internal/http/handlers_upload_test.go
echo "== protocol v2 header occurrences =="
rg -n -C 3 'Git-Protocol|protocol=v2|info/refs' internal/http handlers.go internal/http/handlers_*test.go README.md 2>/dev/null || trueRepository: livrasand/gitGost
Length of output: 21039
🌐 Web query:
Git HTTP protocol v2 client sends GET /info/refs service=git-upload-pack Git-Protocol header
💡 Result:
In the Git wire protocol version 2, a client initiates a request to use v2 over HTTP or HTTPS by including the Git-Protocol header with the value version=2 in its initial smart info/refs request [1][2][3]. The client makes a GET request to the URL path /info/refs with the service query parameter set to the desired service (typically git-upload-pack for fetches or git-receive-pack for pushes) [1][4][5]. The full request structure is as follows: C: GET $GIT_URL/info/refs?service=git-upload-pack HTTP/1.0 C: Git-Protocol: version=2 Upon receiving this request, if the server supports protocol version 2, it responds with a capability advertisement [3][6][7]. This advertisement begins with a version string (version 2) followed by a list of available capabilities [3][6]. The Git-Protocol header acts as a side-channel mechanism for the client to negotiate the protocol version [6][7]. On the server side, the web server or CGI script (such as git-http-backend) must be configured to pass the contents of this header into the environment variable GIT_PROTOCOL so that the Git backend processes can access the requested protocol version [4][3]. After this initial handshake, subsequent requests are directed to the service endpoint, such as $GIT_URL/git-upload-pack, where the client can then issue specific commands like ls-refs or fetch [1][3].
Citations:
- 1: https://git-scm.com/docs/gitprotocol-v2
- 2: https://github.com/git/git/blob/v2.54.0/Documentation/gitprotocol-v2.adoc
- 3: https://git-scm.com/docs/gitprotocol-v2/2.47.1
- 4: https://git-scm.com/docs/gitprotocol-v2/2.38.0
- 5: https://git-scm.com/docs/gitprotocol-http/2.53.0
- 6: https://public-inbox.org/git/20171024183557.GB79163@google.com/T/
- 7: https://git-scm.com/docs/protocol-v2/2.18.0
Forward Git-Protocol during discovery.
The initial GET /info/refs?service=git-upload-pack for protocol v2 uses the Git-Protocol: version=2 header, not protocol=v2 as a query parameter. UploadPackDiscoveryHandler copies query params but does not copy this header, while the later POST handler does; the later header cannot change the advertisement already returned. Add the header forwarding to the discovery request and cover it in the discovery proxy test.
🤖 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/handlers.go` around lines 663 - 676, The
UploadPackDiscoveryHandler discovery proxy must forward the client’s
Git-Protocol header to the remote request, since query parameters do not select
protocol v2. Update the remote discovery request construction around remoteURL
to propagate the incoming Git-Protocol value, and extend the discovery proxy
test to verify this header is forwarded.
| "401", | ||
| "403", | ||
| "404", |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Anchor the HTTP status codes to avoid misclassifying transient failures.
isRetryable matches bare "401", "403", "404", "502", "503", and "504" anywhere in the message. The message that reaches this function is the last stderr line captured by runGitCapture, which often contains counters, ports, byte sizes, and URLs. For example Receiving objects: 50% (404/808) or a host …:4040 matches "404". Because noRetry is evaluated first, such a match turns a transient network failure into a permanent failure, and the job fails without any retry.
Match the codes together with their surrounding git or curl text.
🐛 Proposed fix
noRetry := []string{
"not found",
"repository not found",
"does not appear to be a git repository",
"authentication failed",
"access denied",
"permission denied",
"already exists",
"denied",
"invalid refspec",
"couldn't find remote ref",
- "401",
- "403",
- "404",
+ "error: 401",
+ "error: 403",
+ "error: 404",
+ "http 401",
+ "http 403",
+ "http 404",
}
@@
- "502",
- "503",
- "504",
+ "error: 502",
+ "error: 503",
+ "error: 504",
+ "http 502",
+ "http 503",
+ "http 504",
"http error",
}The existing test cases in internal/jobs/retry_test.go still pass with this change, because they use the HTTP 502 and returned error: 504 forms.
Also applies to: 62-64
🤖 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/retry.go` around lines 34 - 36, Update isRetryable’s noRetry
status matching for 401, 403, 404, 502, 503, and 504 to require surrounding git
or curl HTTP-error text rather than matching bare numeric substrings. Preserve
recognition of existing forms such as “HTTP 502” and “returned error: 504”,
while preventing counters, ports, URLs, or other incidental numbers from being
classified as permanent failures.
| func runGitCapture(dir string, progress func(string), args ...string) error { | ||
| cmd := exec.Command("git", args...) | ||
| cmd.Dir = dir |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Force a stable locale for the git commands. isRetryable matches English substrings such as "could not resolve host" and "repository not found". Neither command builder sets the child environment, so git emits localized messages when the user's environment defines a non-English locale. Every retry decision then falls through to return false, and transient network failures fail the job immediately. The shared root cause is the missing environment control on the two exec.Command("git", …) builders.
internal/jobs/retry.go#L102-L104: setcmd.Env = append(os.Environ(), "LC_ALL=C", "LANG=C")inrunGitCapturebeforecmd.Start().internal/jobs/clone.go#L153-L156: set the samecmd.EnvingitOutputbeforecmd.CombinedOutput(), so the wrapped stderr message that reachesisRetryableis also in English.
📍 Affects 2 files
internal/jobs/retry.go#L102-L104(this comment)internal/jobs/clone.go#L153-L156
🤖 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/retry.go` around lines 102 - 104, Force a stable English locale
for both git command builders by setting cmd.Env to the existing process
environment plus LC_ALL=C and LANG=C. Apply this in runGitCapture in
internal/jobs/retry.go at lines 102-104 before cmd.Start(), and in gitOutput in
internal/jobs/clone.go at lines 153-156 before cmd.CombinedOutput().
| if err := scanner.Err(); err != nil { | ||
| // Si el stderr deja de leerse, git puede bloquearse en el pipe: se aborta. | ||
| _ = cmd.Process.Kill() | ||
| return err | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Call cmd.Wait() after killing the process.
If scanner.Err() returns an error, the code kills the child and returns without calling cmd.Wait(). os/exec requires Wait to reap the child and release the pipe held by StderrPipe. Without it the killed git process stays as a zombie and the pipe file descriptor leaks for the lifetime of the caller. Run and the retry loop can invoke this repeatedly, so the leak accumulates across retries.
🐛 Proposed fix
if err := scanner.Err(); err != nil {
// Si el stderr deja de leerse, git puede bloquearse en el pipe: se aborta.
_ = cmd.Process.Kill()
+ _ = cmd.Wait()
return err
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if err := scanner.Err(); err != nil { | |
| // Si el stderr deja de leerse, git puede bloquearse en el pipe: se aborta. | |
| _ = cmd.Process.Kill() | |
| return err | |
| } | |
| if err := scanner.Err(); err != nil { | |
| // Si el stderr deja de leerse, git puede bloquearse en el pipe: se aborta. | |
| _ = cmd.Process.Kill() | |
| _ = cmd.Wait() | |
| return 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/jobs/retry.go` around lines 129 - 133, Update the scanner.Err()
failure path to call cmd.Wait() after cmd.Process.Kill() and before returning
the scanner error, ensuring the child process is reaped and StderrPipe resources
are released.
| if job.State == StateCompleted || job.State == StateFailed { | ||
| return fmt.Errorf("el job %d ya terminó (%s)", id, job.State) | ||
| } | ||
| if err := s.SetState(id, StateRunning); err != nil { | ||
| return err | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Claim the job atomically to prevent two runners on the same job.
The check at line 19 rejects StateCompleted and StateFailed, but it accepts StateRunning. The check and the SetState at line 22 are also separate statements, so two concurrent git gost run <id> invocations can both pass the check. Both then execute git in the same target directory, which produces index.lock failures or a corrupted working tree.
Replace the read-then-write sequence with a single conditional UPDATE and verify RowsAffected.
🔒 Proposed fix
Add a claim method to internal/jobs/store.go:
// Claim marca el job como running solo si no está terminado ni en ejecución.
func (s *Store) Claim(id int64) (bool, error) {
res, err := s.db.Exec(
`UPDATE jobs SET state = ?, updated_at = ?
WHERE id = ? AND state NOT IN (?, ?, ?)`,
StateRunning, time.Now().UTC().Format(time.RFC3339Nano),
id, StateRunning, StateCompleted, StateFailed)
if err != nil {
return false, err
}
n, err := res.RowsAffected()
return n == 1, err
}Then use it in Run:
- if job.State == StateCompleted || job.State == StateFailed {
- return fmt.Errorf("el job %d ya terminó (%s)", id, job.State)
- }
- if err := s.SetState(id, StateRunning); err != nil {
- return err
- }
+ claimed, err := s.Claim(id)
+ if err != nil {
+ return err
+ }
+ if !claimed {
+ return fmt.Errorf("el job %d no es ejecutable (%s)", id, job.State)
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if job.State == StateCompleted || job.State == StateFailed { | |
| return fmt.Errorf("el job %d ya terminó (%s)", id, job.State) | |
| } | |
| if err := s.SetState(id, StateRunning); err != nil { | |
| return err | |
| } | |
| claimed, err := s.Claim(id) | |
| if err != nil { | |
| return err | |
| } | |
| if !claimed { | |
| return fmt.Errorf("el job %d no es ejecutable (%s)", id, job.State) | |
| } |
🤖 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/run.go` around lines 19 - 24, Replace the separate state
validation and SetState calls in Run with an atomic claim through a new
Store.Claim method. Implement Claim in internal/jobs/store.go using a
conditional UPDATE that excludes Running, Completed, and Failed states, then
verify RowsAffected; have Run return the existing terminal-state error when the
claim is unsuccessful and propagate database errors.
2 participants
Introduce a full client CLI and local job queue: adds cmd/gost main and internal/cli (install, clone, jobs, watch, pause/resume/cancel, rewrite rules, platform process handling). Adds internal/jobs (store backed by modernc.org/sqlite, run/retry logic, layered clone with resume, unit tests). Updates internal HTTP handlers to preserve protocol/query params and increase timeouts. Adds web UI content for the Git extension and a GitHub Actions workflow to build and publish platform binaries with SHA/Sigstore attestations. Also updates go.mod/go.sum for new deps and test utilities.
Summary by CodeRabbit
New Features
git-gostcommand-line client for cloning, Git operations, job management, monitoring, pausing, resuming, and cancellation.Bug Fixes