Feat/v1/apply patches#12
Merged
Merged
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughThis PR adds patch-directory setup, shared git command helpers, patch apply/revert operations, and service wiring so apply and revert now process patch files before overlook handling. ChangesPatches setup and application
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant ApplyRun as apply.Run
participant Validate as filesystem.ValidatePatchesSetup
participant ApplyPatches as git.ApplyPatches
participant GitCLI as git command
participant RevertRun as revert.Run
participant RevertPatches as git.RevertPatches
ApplyRun->>Validate: validate patch setup
Validate-->>ApplyRun: ok / error
ApplyRun->>ApplyPatches: apply patch files
loop each patch file
ApplyPatches->>GitCLI: git apply --check
GitCLI-->>ApplyPatches: success / failure
ApplyPatches->>GitCLI: git apply
GitCLI-->>ApplyPatches: success / failure
end
ApplyPatches-->>ApplyRun: nil
RevertRun->>Validate: validate patch setup
Validate-->>RevertRun: ok / error
RevertRun->>RevertPatches: revert patch files
loop each patch file
RevertPatches->>GitCLI: git apply --check --reverse
GitCLI-->>RevertPatches: success / failure
RevertPatches->>GitCLI: git apply --reverse
GitCLI-->>RevertPatches: success / failure
end
RevertPatches-->>RevertRun: nil
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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: 2
🧹 Nitpick comments (1)
internal/git/utils.go (1)
9-9: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winUse
exec.CommandContextper lint hint.golangci-lint's
noctxrule flags bareexec.Command. Since this is now a shared helper invoked from multiple call sites (including patch generation), adding a context with a sane timeout would also guard against hung git processes.🤖 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/utils.go` at line 9, The shared git helper currently uses a bare exec.Command, which triggers the noctx lint and can hang without cancellation. Update the command निर्माण in the git utility helper to use exec.CommandContext with a context that has a reasonable timeout, and thread that context through the helper’s callers so git invocations can be cancelled cleanly. Keep the fix localized around the helper that builds the git command so all call sites benefit, including patch generation.Source: Linters/SAST tools
🤖 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/filesystem/initialization.go`:
- Around line 119-128: In ValidatePatchesSetup, the error returned by the
os.Stat/config.PATCHES_DIR check violates ST1005 because the string ends with
newline/punctuation and starts with a capitalized sentence. Update the
fmt.Errorf message to use lowercase, remove the trailing newline characters, and
keep it as a clean wrapped error string. Make sure the fix is applied in the
ValidatePatchesSetup function without changing the validation logic.
In `@internal/git/utils.go`:
- Around line 8-18: The shared executeGitCommand helper is returning a hardcoded
“Failed to run Skip Worktree” error, which is misleading for callers like
isFileTracked and future git usages from Generate. Update executeGitCommand to
accept a caller-specific operation/context string and use that in the error
returned from cmd.CombinedOutput, so the message matches the actual git command
being run instead of always mentioning Skip Worktree.
---
Nitpick comments:
In `@internal/git/utils.go`:
- Line 9: The shared git helper currently uses a bare exec.Command, which
triggers the noctx lint and can hang without cancellation. Update the command
निर्माण in the git utility helper to use exec.CommandContext with a context that
has a reasonable timeout, and thread that context through the helper’s callers
so git invocations can be cancelled cleanly. Keep the fix localized around the
helper that builds the git command so all call sites benefit, including patch
generation.
🪄 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: ffcb82d5-5b49-4eae-8300-e73dafe2b137
📒 Files selected for processing (7)
cmd/apply.gointernal/config/constants.gointernal/filesystem/initialization.gointernal/git/patches.gointernal/git/utils.gointernal/git/worktrees.gointernal/service/apply/applicator.go
💤 Files with no reviewable changes (1)
- internal/git/worktrees.go
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/git/patches.go (1)
28-40: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
ApplyPatchesalways returnsnil, silently swallowing failures.Both
checkPatchValidityandpatchFilefailures are only logged withfmt.Printf; the loop alwayscontinues or falls through, and the function unconditionally returnsnilat Line 39. This makes theerrorreturn type misleading — the caller'sif err := git.ApplyPatches(patchFiles); err != nil { return err }ininternal/service/apply/applicator.go(Line 23) is effectively dead code. Any patch that fails to apply or fails--checkvalidation will be silently ignored, andRun()will continue on toapplyOverlookand print a completion message despite the failure.🐛 Proposed fix: propagate patch errors
+import "errors" + func ApplyPatches(filesToPatch []string) error { + var errs []error for _, filePath := range filesToPatch { if err := checkPatchValidity(filePath); err != nil { fmt.Printf("\t[Error] while trying to patch file %s : %s\n", filePath, err.Error()) + errs = append(errs, err) continue } if err := patchFile(filePath); err != nil { fmt.Printf("\t[Error] while trying to patch file %s : %s\n", filePath, err.Error()) + errs = append(errs, err) } } - return nil + return errors.Join(errs...) }🤖 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/patches.go` around lines 28 - 40, `ApplyPatches` currently logs `checkPatchValidity` and `patchFile` failures but always returns nil, so callers like the one in `applicator.go` never see patch errors. Update `ApplyPatches` to propagate failures instead of continuing silently: keep the first error (or wrap with file context) and return it after the loop, and only continue when that behavior is explicitly intended. Use the existing `checkPatchValidity` and `patchFile` calls in `internal/git/patches.go` to ensure any validation or apply failure causes the function’s error return to be non-nil.
🧹 Nitpick comments (1)
internal/git/utils.go (1)
9-9: 🩺 Stability & Availability | 🔵 Trivial | ⚖️ Poor tradeoffConsider
exec.CommandContextinstead ofexec.Command.Static analysis flags this as missing a context/timeout, which could let a hung
gitsubprocess block the caller indefinitely on this shared helper.🤖 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/utils.go` at line 9, The git command helper currently uses exec.Command without any cancellation or timeout, which can leave a hung subprocess blocking callers indefinitely. Update the shared helper in internal/git/utils.go to accept and use a context, and switch the command creation at the cmd := exec.Command("git", args...) call site to exec.CommandContext so callers can enforce cancellation and timeouts; keep the rest of the git invocation behavior unchanged.Source: Linters/SAST tools
🤖 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/service/apply/applicator.go`:
- Line 19: The warning message in applicator.go contains typos in the string
printed by the apply logic. Update the message in the relevant print statement
so “patche” becomes “patch” and “devloca” becomes “devlocal”, keeping the rest
of the warning text unchanged.
---
Outside diff comments:
In `@internal/git/patches.go`:
- Around line 28-40: `ApplyPatches` currently logs `checkPatchValidity` and
`patchFile` failures but always returns nil, so callers like the one in
`applicator.go` never see patch errors. Update `ApplyPatches` to propagate
failures instead of continuing silently: keep the first error (or wrap with file
context) and return it after the loop, and only continue when that behavior is
explicitly intended. Use the existing `checkPatchValidity` and `patchFile` calls
in `internal/git/patches.go` to ensure any validation or apply failure causes
the function’s error return to be non-nil.
---
Nitpick comments:
In `@internal/git/utils.go`:
- Line 9: The git command helper currently uses exec.Command without any
cancellation or timeout, which can leave a hung subprocess blocking callers
indefinitely. Update the shared helper in internal/git/utils.go to accept and
use a context, and switch the command creation at the cmd := exec.Command("git",
args...) call site to exec.CommandContext so callers can enforce cancellation
and timeouts; keep the rest of the git invocation behavior unchanged.
🪄 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: a7d6cc94-5ab1-4129-ab98-58b20da201e6
📒 Files selected for processing (3)
internal/git/patches.gointernal/git/utils.gointernal/service/apply/applicator.go
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
1 participant
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.
Summary by CodeRabbit
New Features
Bug Fixes