Conversation
bash /tmp/v090/audit.sh . → verdict: COMPLIANT (all 7 dimensions zero). Co-authored-by: Codex <noreply@openai.com> Co-Authored-By: Virgil <virgil@lethean.io>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughMigrates the repository into a go/ submodule, converts APIs and internals from context.Context/error to core.Context/core.Result, adds CI/Sonar and linting configs, and substantially rewrites tests, examples and CLI drivers to use the core-based framework and iterator/result patterns. Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI / Tests
participant Core as core.Context/core.Result
participant Service as git.Service
participant Lib as go/git package
participant Git as git CLI
participant Remote as Remote Repo
CLI->>Core: create core.Context
CLI->>Service: call Service action (e.g. PushMultiple) with ctx
Service->>Core: validate paths (core.PathIsAbs/Clean)
Service->>Lib: invoke PushMultiple(ctx, paths, names)
Lib->>Core: build gitCmd and run git process
Lib->>Git: execute git push/fetch
Git->>Remote: network interaction (push/pull)
Git-->>Lib: stdout/stderr, exit code
Lib->>Core: return core.Ok or core.Fail (normalize GitError)
Service->>Core: aggregate per-repo results into core.Result
CLI-->>Core: inspect Result.OK and print/exit
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Review rate limit: 4/5 reviews remaining, refill in 12 minutes. Comment |
Audit verdict: COMPLIANT across all 24 dimensions. Notable changes: - Production Result/error shape rewritten in git.go + service.go - CLI helper rebuilt to avoid banned imports - AX-7 triplet tests rebuilt in sibling test files - service_extra_test.go removed - Examples authored for every public symbol - AGENTS.md authored Co-authored-by: Codex <noreply@openai.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.woodpecker.yml:
- Line 3: The CI config is referencing an admin-scoped Sonar token (sonar_token)
which is overly privileged; replace it with a project-scoped Sonar token that
has analysis-only permissions, update the Woodpecker secret named sonar_token on
the server to the new project-scoped token, and ensure the pipeline continues
using the same secret name so no other pipeline changes are needed; also
rotate/revoke the old admin-scoped token to remove excess privilege.
In `@service_test.go`:
- Around line 47-298: The test function names use the incorrect pattern
"TestService_Service_<Symbol>_<Case>" (e.g., TestService_Service_OnStartup_Good,
TestService_Service_Status_Bad, TestService_Service_All_Ugly,
TestService_Service_DirtyRepos_Good, TestService_Service_BehindRepos_Ugly) and
must be renamed to follow the AX-7 convention
"TestService_<Symbol>_{Good,Bad,Ugly}" (e.g., TestService_OnStartup_Good,
TestService_Status_Bad, TestService_All_Ugly, TestService_DirtyRepos_Good,
TestService_BehindRepos_Ugly); update each test declaration and any references
to them accordingly while keeping the test bodies and referenced symbols
(Service.OnStartup, Service.Status, Service.All, Service.Dirty, Service.Ahead,
Service.Behind, Service.DirtyRepos, Service.AheadRepos, Service.BehindRepos)
unchanged.
- Around line 82-298: Replace tests that construct svc via testService(...) and
directly mutate svc.lastStatus with tests that create real git repositories
using initTestRepo() in temp dirs and exercise the Service methods (Status, All,
Dirty, Ahead, Behind, DirtyRepos, AheadRepos, BehindRepos) against those repos;
specifically, instead of calling testService(...) or setting svc.lastStatus
(symbols: testService, svc.lastStatus, RepoStatus), call initTestRepo() to
create repos with the desired state (clean, modified, staged, ahead, behind,
errored) and pass their paths to the Service constructor or loader so the
Service computes real statuses, then assert on the returned values and iterators
(Status, All, Dirty, Ahead, Behind or their repo list helpers) and remove tests
that rely on mutating lastStatus to simulate races.
- Around line 27-36: The test TestService_NewService_Bad currently treats a
relative WorkDir as valid; update it to assert the bad-path instead: call
NewService with ServiceOptions{WorkDir: "relative/workdir"} and assert r.OK is
false (e.g., AssertFalse(t, r.OK)), assert r.Error or r.Value is nil / no
Service was created, and verify the error message references WorkDir must be
absolute or validation failure. Ensure this aligns with the Service/NewService
implementation which should call validatePath on opts.WorkDir to enforce
absolute paths.
In `@service.go`:
- Around line 149-152: Cast the incoming QueryStatus value to StatusOptions
first to preserve the identical shape contract, then set the resolved fields on
that struct before calling Status; specifically, convert the QueryStatus
instance (the one named or passed into the handler) into StatusOptions and then
assign Paths = resolved and Names = resolvedNames(m.Paths, resolved, m.Names)
before invoking Status(ctx, <that StatusOptions>), rather than constructing a
new StatusOptions literal directly.
- Around line 154-160: The code stores the mutable slice variable `statuses`
into `s.lastStatus` while returning the same backing slice, allowing external
callers to mutate internal state; fix by making defensive copies of the slice:
create a new slice copy when assigning to `s.lastStatus` and return a separate
copy (not the stored slice) from the function so the cached slice under
`statusLock` is never exposed; use the same
`statusLock`/`c.Lock(statusLockName)` protection when updating `s.lastStatus`
and ensure the returned value is a newly allocated slice derived from `statuses`
(or from the stored copy) to eliminate aliasing.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: fee3675c-4697-4ae0-b581-21c57aefeb27
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (12)
.woodpecker.ymlAGENTS.mdgit.gogit_example_test.gogit_test.gogo.modservice.goservice_example_test.goservice_extra_test.goservice_test.gosonar-project.propertiestests/cli/git/main.go
💤 Files with no reviewable changes (1)
- service_extra_test.go
| @@ -0,0 +1,37 @@ | |||
| # Woodpecker CI pipeline. | |||
| # Server: ci.lthn.sh. Lint + sonar in parallel, both depend only on clone. | |||
| # sonar_token is admin-scoped on the Woodpecker server. | |||
There was a problem hiding this comment.
Reduce Sonar token privilege scope.
The comment confirms an admin-scoped token is used for CI scanning. That is broader than required and increases blast radius if CI secrets are exposed. Use a project-scoped token with analysis-only permissions.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.woodpecker.yml at line 3, The CI config is referencing an admin-scoped
Sonar token (sonar_token) which is overly privileged; replace it with a
project-scoped Sonar token that has analysis-only permissions, update the
Woodpecker secret named sonar_token on the server to the new project-scoped
token, and ensure the pipeline continues using the same secret name so no other
pipeline changes are needed; also rotate/revoke the old admin-scoped token to
remove excess privilege.
Comment on lines
+27
to
36
| func TestService_NewService_Bad(t *T) { | ||
| c := New() | ||
| opts := ServiceOptions{WorkDir: "relative/workdir"} | ||
|
|
||
| dirty := s.DirtyRepos() | ||
| if len(dirty) != 0 { | ||
| t.Fatalf("want %v, got %v", 0, len(dirty)) | ||
| } | ||
| r := NewService(opts)(c) | ||
|
|
||
| AssertTrue(t, r.OK) | ||
| svc := r.Value.(*Service) | ||
| AssertEqual(t, "relative/workdir", svc.opts.WorkDir) | ||
| } |
There was a problem hiding this comment.
NewService bad-path case is asserting the opposite of the path policy.
This test currently treats a relative WorkDir as valid. The bad-path case should assert failure to prevent locking in non-compliant behaviour.
Proposed test expectation update
func TestService_NewService_Bad(t *T) {
c := New()
opts := ServiceOptions{WorkDir: "relative/workdir"}
r := NewService(opts)(c)
- AssertTrue(t, r.OK)
- svc := r.Value.(*Service)
- AssertEqual(t, "relative/workdir", svc.opts.WorkDir)
+ AssertFalse(t, r.OK)
+ AssertContains(t, r.Error(), "path must be absolute")
}As per coding guidelines: All paths must be absolute; Service must enforce WorkDir boundary via validatePath.
📝 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.
Suggested change
| func TestService_NewService_Bad(t *T) { | |
| c := New() | |
| opts := ServiceOptions{WorkDir: "relative/workdir"} | |
| dirty := s.DirtyRepos() | |
| if len(dirty) != 0 { | |
| t.Fatalf("want %v, got %v", 0, len(dirty)) | |
| } | |
| r := NewService(opts)(c) | |
| AssertTrue(t, r.OK) | |
| svc := r.Value.(*Service) | |
| AssertEqual(t, "relative/workdir", svc.opts.WorkDir) | |
| } | |
| func TestService_NewService_Bad(t *T) { | |
| c := New() | |
| opts := ServiceOptions{WorkDir: "relative/workdir"} | |
| r := NewService(opts)(c) | |
| AssertFalse(t, r.OK) | |
| AssertContains(t, r.Error(), "path must be absolute") | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@service_test.go` around lines 27 - 36, The test TestService_NewService_Bad
currently treats a relative WorkDir as valid; update it to assert the bad-path
instead: call NewService with ServiceOptions{WorkDir: "relative/workdir"} and
assert r.OK is false (e.g., AssertFalse(t, r.OK)), assert r.Error or r.Value is
nil / no Service was created, and verify the error message references WorkDir
must be absolute or validation failure. Ensure this aligns with the
Service/NewService implementation which should call validatePath on opts.WorkDir
to enforce absolute paths.
Comment on lines
-47
to
298
| t.Fatalf("expected true, got %v", result.Value) | ||
| } | ||
| pulls, ok := result.Value.([]PullResult) | ||
| if !ok { | ||
| t.Fatalf("expected []PullResult, got %T", result.Value) | ||
| } | ||
| if len(pulls) != 0 { | ||
| t.Fatalf("want %v, got %v", 0, len(pulls)) | ||
| } | ||
| ahead := svc.AheadRepos() | ||
|
|
||
| AssertLen(t, ahead, 2) | ||
| AssertContains(t, repoNames(ahead), "ahead-one") | ||
| AssertContains(t, repoNames(ahead), "ahead-two") | ||
| } | ||
|
|
||
| func TestService_TaskPullMultiple_Bad(t *testing.T) { | ||
| s := &Service{} | ||
| func TestService_Service_AheadRepos_Bad(t *T) { | ||
| svc := testService(RepoStatus{Name: "ahead-error", Ahead: 1, Error: NewError("status failed")}) | ||
|
|
||
| result := s.runPullMultiple(context.Background(), []string{"relative/path"}, nil) | ||
| if result.OK { | ||
| t.Fatal("expected false") | ||
| } | ||
| err, ok := result.Value.(error) | ||
| if !ok { | ||
| t.Fatalf("expected error, got %T", result.Value) | ||
| } | ||
| var gitErr *GitError | ||
| if !errors.As(err, &gitErr) { | ||
| t.Fatalf("expected *GitError, got %T", err) | ||
| } | ||
| if !slices.Contains(gitErr.Args, "path=relative/path") { | ||
| t.Fatalf("expected args %v to contain relative path", gitErr.Args) | ||
| } | ||
| } | ||
| ahead := svc.AheadRepos() | ||
|
|
||
| func TestService_TaskPullMultiple_Ugly(t *testing.T) { | ||
| task := TaskPullMultiple{ | ||
| Paths: []string{}, | ||
| Names: map[string]string{}, | ||
| } | ||
| AssertLen(t, ahead, 0) | ||
| } | ||
|
|
||
| if task.Paths == nil { | ||
| t.Fatal("expected empty but non-nil paths") | ||
| } | ||
| if task.Names == nil { | ||
| t.Fatal("expected empty but non-nil names") | ||
| } | ||
| func TestService_Service_AheadRepos_Ugly(t *T) { | ||
| svc := testService() | ||
| ahead := svc.AheadRepos() | ||
| AssertLen(t, ahead, 0) | ||
| AssertNil(t, ahead) | ||
| } | ||
|
|
||
| func TestService_ServiceOptions_Good(t *testing.T) { | ||
| workDir := "/tmp/test-repos" | ||
| opts := ServiceOptions{WorkDir: workDir} | ||
| if workDir != opts.WorkDir { | ||
| t.Fatalf("want %v, got %v", workDir, opts.WorkDir) | ||
| } | ||
| func TestService_Service_BehindRepos_Good(t *T) { | ||
| svc := testService(RepoStatus{Name: "synced"}, RepoStatus{Name: "behind-one", Behind: 1}, RepoStatus{Name: "behind-two", Behind: 2}) | ||
|
|
||
| behind := svc.BehindRepos() | ||
|
|
||
| AssertLen(t, behind, 2) | ||
| AssertContains(t, repoNames(behind), "behind-one") | ||
| AssertContains(t, repoNames(behind), "behind-two") | ||
| } | ||
|
|
||
| func TestService_ServiceOptions_Bad(t *testing.T) { | ||
| opts := ServiceOptions{WorkDir: "relative/workdir"} | ||
| if opts.WorkDir != "relative/workdir" { | ||
| t.Fatalf("want %v, got %v", "relative/workdir", opts.WorkDir) | ||
| } | ||
| func TestService_Service_BehindRepos_Bad(t *T) { | ||
| svc := testService(RepoStatus{Name: "behind-error", Behind: 1, Error: NewError("status failed")}) | ||
|
|
||
| behind := svc.BehindRepos() | ||
|
|
||
| AssertLen(t, behind, 0) | ||
| } | ||
|
|
||
| func TestService_ServiceOptions_Ugly(t *testing.T) { | ||
| var opts ServiceOptions | ||
| if opts.WorkDir != "" { | ||
| t.Fatalf("want empty WorkDir, got %v", opts.WorkDir) | ||
| } | ||
| func TestService_Service_BehindRepos_Ugly(t *T) { | ||
| svc := testService() | ||
| behind := svc.BehindRepos() | ||
| AssertLen(t, behind, 0) | ||
| AssertNil(t, behind) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy lift
Test names do not follow the required AX-7 service naming pattern.
Most methods are named TestService_Service_<Symbol>_...; they should be TestService_<Symbol>_{Good,Bad,Ugly} to match the enforced convention.
As per coding guidelines: **/service_test.go: Every public service method in service.go is tested in service_test.go with the matching TestService_<Symbol>_{Good,Bad,Ugly} naming convention.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@service_test.go` around lines 47 - 298, The test function names use the
incorrect pattern "TestService_Service_<Symbol>_<Case>" (e.g.,
TestService_Service_OnStartup_Good, TestService_Service_Status_Bad,
TestService_Service_All_Ugly, TestService_Service_DirtyRepos_Good,
TestService_Service_BehindRepos_Ugly) and must be renamed to follow the AX-7
convention "TestService_<Symbol>_{Good,Bad,Ugly}" (e.g.,
TestService_OnStartup_Good, TestService_Status_Bad, TestService_All_Ugly,
TestService_DirtyRepos_Good, TestService_BehindRepos_Ugly); update each test
declaration and any references to them accordingly while keeping the test bodies
and referenced symbols (Service.OnStartup, Service.Status, Service.All,
Service.Dirty, Service.Ahead, Service.Behind, Service.DirtyRepos,
Service.AheadRepos, Service.BehindRepos) unchanged.
Comment on lines
-82
to
298
| t.Fatalf("expected true, got %v", result.Value) | ||
| } | ||
| pulls, ok := result.Value.([]PullResult) | ||
| if !ok { | ||
| t.Fatalf("expected []PullResult, got %T", result.Value) | ||
| } | ||
| if len(pulls) != 0 { | ||
| t.Fatalf("want %v, got %v", 0, len(pulls)) | ||
| } | ||
| ahead := svc.AheadRepos() | ||
|
|
||
| AssertLen(t, ahead, 2) | ||
| AssertContains(t, repoNames(ahead), "ahead-one") | ||
| AssertContains(t, repoNames(ahead), "ahead-two") | ||
| } | ||
|
|
||
| func TestService_TaskPullMultiple_Bad(t *testing.T) { | ||
| s := &Service{} | ||
| func TestService_Service_AheadRepos_Bad(t *T) { | ||
| svc := testService(RepoStatus{Name: "ahead-error", Ahead: 1, Error: NewError("status failed")}) | ||
|
|
||
| result := s.runPullMultiple(context.Background(), []string{"relative/path"}, nil) | ||
| if result.OK { | ||
| t.Fatal("expected false") | ||
| } | ||
| err, ok := result.Value.(error) | ||
| if !ok { | ||
| t.Fatalf("expected error, got %T", result.Value) | ||
| } | ||
| var gitErr *GitError | ||
| if !errors.As(err, &gitErr) { | ||
| t.Fatalf("expected *GitError, got %T", err) | ||
| } | ||
| if !slices.Contains(gitErr.Args, "path=relative/path") { | ||
| t.Fatalf("expected args %v to contain relative path", gitErr.Args) | ||
| } | ||
| } | ||
| ahead := svc.AheadRepos() | ||
|
|
||
| func TestService_TaskPullMultiple_Ugly(t *testing.T) { | ||
| task := TaskPullMultiple{ | ||
| Paths: []string{}, | ||
| Names: map[string]string{}, | ||
| } | ||
| AssertLen(t, ahead, 0) | ||
| } | ||
|
|
||
| if task.Paths == nil { | ||
| t.Fatal("expected empty but non-nil paths") | ||
| } | ||
| if task.Names == nil { | ||
| t.Fatal("expected empty but non-nil names") | ||
| } | ||
| func TestService_Service_AheadRepos_Ugly(t *T) { | ||
| svc := testService() | ||
| ahead := svc.AheadRepos() | ||
| AssertLen(t, ahead, 0) | ||
| AssertNil(t, ahead) | ||
| } | ||
|
|
||
| func TestService_ServiceOptions_Good(t *testing.T) { | ||
| workDir := "/tmp/test-repos" | ||
| opts := ServiceOptions{WorkDir: workDir} | ||
| if workDir != opts.WorkDir { | ||
| t.Fatalf("want %v, got %v", workDir, opts.WorkDir) | ||
| } | ||
| func TestService_Service_BehindRepos_Good(t *T) { | ||
| svc := testService(RepoStatus{Name: "synced"}, RepoStatus{Name: "behind-one", Behind: 1}, RepoStatus{Name: "behind-two", Behind: 2}) | ||
|
|
||
| behind := svc.BehindRepos() | ||
|
|
||
| AssertLen(t, behind, 2) | ||
| AssertContains(t, repoNames(behind), "behind-one") | ||
| AssertContains(t, repoNames(behind), "behind-two") | ||
| } | ||
|
|
||
| func TestService_ServiceOptions_Bad(t *testing.T) { | ||
| opts := ServiceOptions{WorkDir: "relative/workdir"} | ||
| if opts.WorkDir != "relative/workdir" { | ||
| t.Fatalf("want %v, got %v", "relative/workdir", opts.WorkDir) | ||
| } | ||
| func TestService_Service_BehindRepos_Bad(t *T) { | ||
| svc := testService(RepoStatus{Name: "behind-error", Behind: 1, Error: NewError("status failed")}) | ||
|
|
||
| behind := svc.BehindRepos() | ||
|
|
||
| AssertLen(t, behind, 0) | ||
| } | ||
|
|
||
| func TestService_ServiceOptions_Ugly(t *testing.T) { | ||
| var opts ServiceOptions | ||
| if opts.WorkDir != "" { | ||
| t.Fatalf("want empty WorkDir, got %v", opts.WorkDir) | ||
| } | ||
| func TestService_Service_BehindRepos_Ugly(t *T) { | ||
| svc := testService() | ||
| behind := svc.BehindRepos() | ||
| AssertLen(t, behind, 0) | ||
| AssertNil(t, behind) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy lift
Service tests should use real git repositories, not only synthetic lastStatus fixtures.
The current suite heavily validates by injecting lastStatus directly. For compliance and behavioural confidence, these scenarios should be exercised using temp repos created via initTestRepo().
As per coding guidelines: **/*_test.go: Tests should use real git repos created by initTestRepo() in temp directories.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@service_test.go` around lines 82 - 298, Replace tests that construct svc via
testService(...) and directly mutate svc.lastStatus with tests that create real
git repositories using initTestRepo() in temp dirs and exercise the Service
methods (Status, All, Dirty, Ahead, Behind, DirtyRepos, AheadRepos, BehindRepos)
against those repos; specifically, instead of calling testService(...) or
setting svc.lastStatus (symbols: testService, svc.lastStatus, RepoStatus), call
initTestRepo() to create repos with the desired state (clean, modified, staged,
ahead, behind, errored) and pass their paths to the Service constructor or
loader so the Service computes real statuses, then assert on the returned values
and iterators (Status, All, Dirty, Ahead, Behind or their repo list helpers) and
remove tests that rely on mutating lastStatus to simulate races.
Comment on lines
149
to
152
| statuses := Status(ctx, StatusOptions{ | ||
| Paths: paths, | ||
| Names: resolvedNames(m.Paths, paths, m.Names), | ||
| Paths: resolved, | ||
| Names: resolvedNames(m.Paths, resolved, m.Names), | ||
| }) |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Cast QueryStatus directly to StatusOptions before applying resolved fields.
This preserves the “same shape” contract and reduces drift risk between these two public types.
Proposed change
- statuses := Status(ctx, StatusOptions{
- Paths: resolved,
- Names: resolvedNames(m.Paths, resolved, m.Names),
- })
+ statusOpts := StatusOptions(m)
+ statusOpts.Paths = resolved
+ statusOpts.Names = resolvedNames(m.Paths, resolved, m.Names)
+ statuses := Status(ctx, statusOpts)As per coding guidelines "QueryStatus and StatusOptions must be structurally identical and cast directly between them".
📝 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.
Suggested change
| statuses := Status(ctx, StatusOptions{ | |
| Paths: paths, | |
| Names: resolvedNames(m.Paths, paths, m.Names), | |
| Paths: resolved, | |
| Names: resolvedNames(m.Paths, resolved, m.Names), | |
| }) | |
| statusOpts := StatusOptions(m) | |
| statusOpts.Paths = resolved | |
| statusOpts.Names = resolvedNames(m.Paths, resolved, m.Names) | |
| statuses := Status(ctx, statusOpts) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@service.go` around lines 149 - 152, Cast the incoming QueryStatus value to
StatusOptions first to preserve the identical shape contract, then set the
resolved fields on that struct before calling Status; specifically, convert the
QueryStatus instance (the one named or passed into the handler) into
StatusOptions and then assign Paths = resolved and Names =
resolvedNames(m.Paths, resolved, m.Names) before invoking Status(ctx, <that
StatusOptions>), rather than constructing a new StatusOptions literal directly.
Comment on lines
154
to
160
| statusLock := c.Lock(statusLockName) | ||
| statusLock.Mutex.Lock() | ||
| s.lastStatus = statuses | ||
| statusLock.Mutex.Unlock() | ||
|
|
||
| return core.Result{Value: statuses, OK: true} | ||
| return core.Ok(statuses) | ||
|
|
There was a problem hiding this comment.
Do not expose the same mutable slice instance that is cached under lock.
Line 156 stores statuses in s.lastStatus, and Line 159 returns the same backing slice. External mutation of the returned value can corrupt internal state outside lock protection.
Proposed change
statusLock := c.Lock(statusLockName)
statusLock.Mutex.Lock()
- s.lastStatus = statuses
+ s.lastStatus = core.SliceClone(statuses)
statusLock.Mutex.Unlock()
- return core.Ok(statuses)
+ return core.Ok(core.SliceClone(statuses))📝 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.
Suggested change
| statusLock := c.Lock(statusLockName) | |
| statusLock.Mutex.Lock() | |
| s.lastStatus = statuses | |
| statusLock.Mutex.Unlock() | |
| return core.Result{Value: statuses, OK: true} | |
| return core.Ok(statuses) | |
| statusLock := c.Lock(statusLockName) | |
| statusLock.Mutex.Lock() | |
| s.lastStatus = core.SliceClone(statuses) | |
| statusLock.Mutex.Unlock() | |
| return core.Ok(core.SliceClone(statuses)) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@service.go` around lines 154 - 160, The code stores the mutable slice
variable `statuses` into `s.lastStatus` while returning the same backing slice,
allowing external callers to mutate internal state; fix by making defensive
copies of the slice: create a new slice copy when assigning to `s.lastStatus`
and return a separate copy (not the stored slice) from the function so the
cached slice under `statusLock` is never exposed; use the same
`statusLock`/`c.Lock(statusLockName)` protection when updating `s.lastStatus`
and ensure the returned value is a newly allocated slice derived from `statuses`
(or from the stored copy) to eliminate aliasing.
Lift Go module into go/ subtree for cross-language repo symmetry (matches
core/api v0.12.0). Module path stays dappco.re/go/git — consumers see no
change. No dappco.re/go/* deps so no go.work or external/ submodules.
Moved into go/: go.mod, go.sum, *.go, tests/.
Stays at repo root: README.md, CLAUDE.md, AGENTS.md, CONTRIBUTING.md,
llms.txt, docs/, sonar-project.properties, .woodpecker.yml.
Symlinks: go/{README,CLAUDE,AGENTS}.md + go/docs → ../
Verification: go mod tidy / vet / test / audit all clean.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
go/service.go (2)
72-72: 💤 Low valueClarify the purpose of string concatenation in
actionPathKey.The construction
"p" + "ath"is unusual. If this is intentional (e.g., to avoid detection by tooling or linting), a brief comment explaining the rationale would help future maintainers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@go/service.go` at line 72, The constant actionPathKey is defined using an odd string concatenation ("p" + "ath") which is unclear; either replace it with a single literal "path" or, if the split is intentional, add a concise comment next to the actionPathKey declaration explaining the reason (e.g., to avoid tooling detection or linter rule) so maintainers understand why it's written that way; locate the actionPathKey constant in the file and either simplify to actionPathKey = "path" or add the explanatory comment above or inline with actionPathKey.
145-145: ⚡ Quick winExtract duplicated
"path validation failed"literal to a constant.This string appears 5 times across path validation error handling. Extracting it improves maintainability.
♻️ Proposed fix
const ( actionGitPush = "git.push" actionGitPull = "git.pull" actionGitPushMultiple = "git.push-multiple" actionGitPullMultiple = "git.pull-multiple" actionPathKey = "p" + "ath" statusLockName = "git.status" + errPathValidationFailed = "path validation failed" )Then replace occurrences:
- return c.LogError(resultError(paths), "git.handleQuery", "path validation failed") + return c.LogError(resultError(paths), "git.handleQuery", errPathValidationFailed)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@go/service.go` at line 145, Extract the repeated literal "path validation failed" into a package-level constant (e.g. const pathValidationFailed = "path validation failed") and replace all occurrences of the literal used in path validation error handling with that constant; specifically update the LogError calls (seen as c.LogError(resultError(paths), "git.handleQuery", "path validation failed")) and any other places that use the same string so they reference pathValidationFailed instead, keeping existing arguments like resultError(paths) and the component string ("git.handleQuery") unchanged.go/tests/cli/git/main.go (2)
242-262: 💤 Low valueExtract duplicated
"relative/repo"literal to a constant.The string
"relative/repo"appears three times inverifyErrors. Extracting it to a constant improves maintainability and aligns with SonarCloud's recommendation.♻️ Proposed fix
+const relativeTestPath = "relative/repo" + func verifyErrors(ctx Context) Result { - statuses := gitlib.Status(ctx, gitlib.StatusOptions{Paths: []string{"relative/repo"}}) + statuses := gitlib.Status(ctx, gitlib.StatusOptions{Paths: []string{relativeTestPath}}) if len(statuses) != 1 || statuses[0].Error == nil { return Fail(NewError("relative status should fail")) } if !Contains(statuses[0].Error.Error(), "path must be absolute") { return Fail(Errorf("relative status error = %v", statuses[0].Error)) } - if r := gitlib.Push(ctx, "relative/repo"); r.OK { + if r := gitlib.Push(ctx, relativeTestPath); r.OK { return Fail(NewError("relative push should fail")) } - if r := gitlib.Pull(ctx, "relative/repo"); r.OK { + if r := gitlib.Pull(ctx, relativeTestPath); r.OK { return Fail(NewError("relative pull should fail")) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@go/tests/cli/git/main.go` around lines 242 - 262, Extract the duplicated string "relative/repo" into a single constant and use it in verifyErrors: add a const (e.g., relativeRepo := "relative/repo") near the top of the function or file, then replace the three literal occurrences passed to gitlib.Status(...Paths...), gitlib.Push(ctx, ...), and gitlib.Pull(ctx, ...) with that constant; update any error messages or checks that reference the same literal to use the constant to avoid duplication and improve maintainability.
131-217: ⚖️ Poor tradeoffConsider splitting
verifyPushPullto reduce cognitive complexity.SonarCloud flags cognitive complexity of 22 (threshold 15). The function performs sequential setup and validation across push/pull scenarios. Extracting sub-functions (e.g.,
setupPushPullRepos,verifyPushFlow,verifyPullFlow,verifyMultipleOps) would improve readability and reduce complexity.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@go/tests/cli/git/main.go` around lines 131 - 217, The verifyPushPull function is too complex (cognitive complexity 22); split its sequential steps into smaller helper functions to reduce complexity: extract repo setup into setupPushPullRepos(ctx) which creates temp dirs, initializes remote, clones push/pull repos, and configures users; extract the local push sequence into verifyPushFlow(ctx, pushClone) to commit v2, check Status via gitlib.Status and expectSingleStatus, call gitlib.Push and re-validate status; extract the fetch/pull sequence into verifyPullFlow(ctx, pullClone) to fetch, check status, call gitlib.Pull and re-validate; extract the batch operations into verifyMultipleOps(ctx, pushClone, pullClone) to call gitlib.PushMultiple and gitlib.PullMultiple and assert results; then have verifyPushPull call these helpers and return their Result values, passing/returning the root temp path cleanup as before. Ensure helper functions reuse existing helpers commitFile, configureUser, runGit, expectSingleStatus and reference gitlib APIs exactly as in the diff.go/git.go (1)
263-263: 💤 Low valueGroup consecutive parameters of the same type.
Static analysis suggests grouping the consecutive
stringparameters for idiomatic Go style.♻️ Suggested fix
-func requireAbsolutePath(op string, path string) core.Result { +func requireAbsolutePath(op, path string) core.Result {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@go/git.go` at line 263, The function signature for requireAbsolutePath currently declares consecutive string params separately; change it to group them as requireAbsolutePath(op, path string) to follow Go idiomatic style while keeping the return type core.Result and preserving the function body and usages.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@go/git.go`:
- Around line 4-8: go/git.go currently depends on the framework via
core.Context, core.Result and many core helpers (core.Ok, core.Fail, core.Split,
core.Lower, core.Contains, core.PathIsAbs, core.Sprintf, etc.); refactor it to
stdlib-only by replacing core.Context and core.Result with native types
(context.Context, native return values/errors or custom small structs), swap
core helpers for standard library equivalents (strings.Split, strings.ToLower,
strings.Contains, path/filepath.IsAbs, fmt.Sprintf, etc.), and convert code
paths that return core.Ok()/core.Fail() to return (value, error) or appropriate
stdlib constructs so all references to the core package are removed from
go/git.go while preserving existing behavior.
---
Nitpick comments:
In `@go/git.go`:
- Line 263: The function signature for requireAbsolutePath currently declares
consecutive string params separately; change it to group them as
requireAbsolutePath(op, path string) to follow Go idiomatic style while keeping
the return type core.Result and preserving the function body and usages.
In `@go/service.go`:
- Line 72: The constant actionPathKey is defined using an odd string
concatenation ("p" + "ath") which is unclear; either replace it with a single
literal "path" or, if the split is intentional, add a concise comment next to
the actionPathKey declaration explaining the reason (e.g., to avoid tooling
detection or linter rule) so maintainers understand why it's written that way;
locate the actionPathKey constant in the file and either simplify to
actionPathKey = "path" or add the explanatory comment above or inline with
actionPathKey.
- Line 145: Extract the repeated literal "path validation failed" into a
package-level constant (e.g. const pathValidationFailed = "path validation
failed") and replace all occurrences of the literal used in path validation
error handling with that constant; specifically update the LogError calls (seen
as c.LogError(resultError(paths), "git.handleQuery", "path validation failed"))
and any other places that use the same string so they reference
pathValidationFailed instead, keeping existing arguments like resultError(paths)
and the component string ("git.handleQuery") unchanged.
In `@go/tests/cli/git/main.go`:
- Around line 242-262: Extract the duplicated string "relative/repo" into a
single constant and use it in verifyErrors: add a const (e.g., relativeRepo :=
"relative/repo") near the top of the function or file, then replace the three
literal occurrences passed to gitlib.Status(...Paths...), gitlib.Push(ctx, ...),
and gitlib.Pull(ctx, ...) with that constant; update any error messages or
checks that reference the same literal to use the constant to avoid duplication
and improve maintainability.
- Around line 131-217: The verifyPushPull function is too complex (cognitive
complexity 22); split its sequential steps into smaller helper functions to
reduce complexity: extract repo setup into setupPushPullRepos(ctx) which creates
temp dirs, initializes remote, clones push/pull repos, and configures users;
extract the local push sequence into verifyPushFlow(ctx, pushClone) to commit
v2, check Status via gitlib.Status and expectSingleStatus, call gitlib.Push and
re-validate status; extract the fetch/pull sequence into verifyPullFlow(ctx,
pullClone) to fetch, check status, call gitlib.Pull and re-validate; extract the
batch operations into verifyMultipleOps(ctx, pushClone, pullClone) to call
gitlib.PushMultiple and gitlib.PullMultiple and assert results; then have
verifyPushPull call these helpers and return their Result values,
passing/returning the root temp path cleanup as before. Ensure helper functions
reuse existing helpers commitFile, configureUser, runGit, expectSingleStatus and
reference gitlib APIs exactly as in the diff.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ac7aa918-a4cf-48eb-bd91-035cf18def63
⛔ Files ignored due to path filters (1)
go/go.sumis excluded by!**/*.sum
📒 Files selected for processing (16)
.woodpecker.ymlCLAUDE.mdgo/AGENTS.mdgo/CLAUDE.mdgo/README.mdgo/docsgo/git.gogo/git_example_test.gogo/git_test.gogo/go.modgo/service.gogo/service_example_test.gogo/service_test.gogo/tests/cli/git/Taskfile.yamlgo/tests/cli/git/main.gosonar-project.properties
✅ Files skipped from review due to trivial changes (5)
- go/CLAUDE.md
- go/docs
- go/go.mod
- sonar-project.properties
- go/AGENTS.md
🚧 Files skipped from review as they are similar to previous changes (1)
- .woodpecker.yml
Comment on lines
4
to
8
| import ( | ||
| "bytes" | ||
| "context" // Note: intrinsic — cancellation propagation for git subprocesses and iterators; no core equivalent | ||
| "fmt" | ||
| "iter" // Note: intrinsic — public lazy sequence API for repository operations; no core equivalent | ||
| "os" // Note: intrinsic — interactive git subprocess standard streams; no core equivalent | ||
| "os/exec" // Note: intrinsic — executing the git CLI for repository operations; no core equivalent | ||
| "path/filepath" | ||
| "slices" // Note: intrinsic — collecting and cloning iterator-backed result slices; no core equivalent | ||
| "strconv" | ||
| "strings" | ||
| "iter" | ||
|
|
||
| core "dappco.re/go" | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if coding guidelines have been updated to reflect new core dependency
fd -i 'CONTRIBUTING.md' --exec cat {}
fd -i 'CLAUDE.md' --exec cat {}Repository: dAppCore/go-git
Length of output: 7540
🏁 Script executed:
# Check the actual imports and their usage in go/git.go
head -50 go/git.go | cat -nRepository: dAppCore/go-git
Length of output: 1479
🏁 Script executed:
# Search for all references to the `core` package in go/git.go
rg -n "core\." go/git.goRepository: dAppCore/go-git
Length of output: 3474
🏁 Script executed:
# Count lines and show the entire file structure
wc -l go/git.go && echo "---" && rg -n "^(func|type|const|var|import)" go/git.go | head -40Repository: dAppCore/go-git
Length of output: 2233
Remove framework dependency from go/git.go to comply with documented architecture.
The CLAUDE.md architecture guide explicitly states that go/git.go should use "stdlib only, no framework dependency", but the current implementation uses the core framework extensively throughout:
- Function signatures rely on
core.Contextandcore.Resulttypes - Core utilities permeate the implementation:
core.Ok(),core.Fail(),core.Split(),core.Lower(),core.Contains(),core.PathIsAbs(),core.Sprintf(), and many others
This violates the documented architectural separation between go/git.go (stdlib only) and go/service.go (framework integration). Refactor core operations to use standard library equivalents.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@go/git.go` around lines 4 - 8, go/git.go currently depends on the framework
via core.Context, core.Result and many core helpers (core.Ok, core.Fail,
core.Split, core.Lower, core.Contains, core.PathIsAbs, core.Sprintf, etc.);
refactor it to stdlib-only by replacing core.Context and core.Result with native
types (context.Context, native return values/errors or custom small structs),
swap core helpers for standard library equivalents (strings.Split,
strings.ToLower, strings.Contains, path/filepath.IsAbs, fmt.Sprintf, etc.), and
convert code paths that return core.Ok()/core.Fail() to return (value, error) or
appropriate stdlib constructs so all references to the core package are removed
from go/git.go while preserving existing behavior.
Mechanical cleanup. No behaviour changes, no API changes. Verified go vet clean, go test passes.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
go/service.go (2)
72-72: 💤 Low valueUnclear purpose of string concatenation for constant.
The concatenation
"p" + "ath"is unusual. If this is intentional (e.g., to avoid static analysis or string matching), add a comment explaining why. Otherwise, simplify to"path".- actionPathKey = "p" + "ath" + actionPathKey = "path"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@go/service.go` at line 72, The constant actionPathKey is defined using an odd string concatenation ("p" + "ath"); either simplify it to a normal literal by replacing actionPathKey = "p" + "ath" with actionPathKey = "path", or if the concatenation is intentional (e.g., to avoid static analysis or pattern matching), add a brief explanatory comment immediately above the declaration mentioning the reason and linking to any relevant issue or test; update the declaration of actionPathKey accordingly so the intent is clear to future readers.
145-145: ⚡ Quick winExtract duplicated literal "path validation failed" to a constant.
SonarCloud flags this literal as duplicated 5 times (lines 145, 195, 207, 219, 235). Define a constant to improve maintainability.
Proposed fix
Add a constant near the other constants:
const ( actionGitPush = "git.push" actionGitPull = "git.pull" actionGitPushMultiple = "git.push-multiple" actionGitPullMultiple = "git.pull-multiple" actionPathKey = "p" + "ath" statusLockName = "git.status" + msgPathValidationFailed = "path validation failed" )Then replace all occurrences, e.g.:
- return c.LogError(resultError(paths), "git.handleQuery", "path validation failed") + return c.LogError(resultError(paths), "git.handleQuery", msgPathValidationFailed)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@go/service.go` at line 145, Introduce a new constant (e.g., pathValidationFailed = "path validation failed") alongside the other file-level constants and replace every hard-coded "path validation failed" literal in this file (including the call that currently passes it to c.LogError in the git handler: c.LogError(resultError(paths), "git.handleQuery", "path validation failed")) with that constant to remove duplication and improve maintainability; ensure all five occurrences flagged by SonarCloud are updated to use the new constant.go/tests/cli/git/main.go (2)
243-262: ⚡ Quick winExtract duplicated literal "relative/repo" to a constant.
SonarCloud flags this literal as duplicated 3 times. Define a constant for maintainability.
Proposed fix
+const relativeRepoPath = "relative/repo" + func verifyErrors(ctx core.Context) core.Result { - statuses := gitlib.Status(ctx, gitlib.StatusOptions{Paths: []string{"relative/repo"}}) + statuses := gitlib.Status(ctx, gitlib.StatusOptions{Paths: []string{relativeRepoPath}}) if len(statuses) != 1 || statuses[0].Error == nil { return core.Fail(core.NewError("relative status should fail")) } if !core.Contains(statuses[0].Error.Error(), "path must be absolute") { return core.Fail(core.Errorf("relative status error = %v", statuses[0].Error)) } - if r := gitlib.Push(ctx, "relative/repo"); r.OK { + if r := gitlib.Push(ctx, relativeRepoPath); r.OK { return core.Fail(core.NewError("relative push should fail")) } - if r := gitlib.Pull(ctx, "relative/repo"); r.OK { + if r := gitlib.Pull(ctx, relativeRepoPath); r.OK { return core.Fail(core.NewError("relative pull should fail")) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@go/tests/cli/git/main.go` around lines 243 - 262, Extract the repeated literal "relative/repo" into a named constant (e.g., const relativeRepo = "relative/repo") near the top of the test and replace the three usages in the block that call gitlib.Status(ctx, gitlib.StatusOptions{Paths: []string{...}}), gitlib.Push(ctx, ...), and gitlib.Pull(ctx, ...) with that constant; ensure references remain unchanged (use the constant in StatusOptions.Paths slice and as the single argument to Push and Pull) and run tests to verify no behavior change.
131-217: ⚖️ Poor tradeoffConsider splitting
verifyPushPullto reduce cognitive complexity.SonarCloud flags this function with cognitive complexity of 22 (allowed: 15). For a test driver, this is acceptable, but splitting into smaller helpers (e.g.,
setupPushPullFixture,verifyPushFlow,verifyPullFlow,verifyMultipleOps) would improve readability.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@go/tests/cli/git/main.go` around lines 131 - 217, The verifyPushPull function is too complex—split it into smaller helpers: create setupPushPullFixture to create the temp repo, remote (remote.git), pushClone and pullClone paths, run initial init/clone/configureUser/initial commit and return the rootPath and clone paths (and ensure defer cleanupTempDir is handled at top-level); implement verifyPushFlow to perform local commit on pushClone, assert statuses via expectSingleStatus and call gitlib.Push and re-check statuses; implement verifyPullFlow to fetch on pullClone, assert statuses, call gitlib.Pull and re-check statuses; implement verifyMultipleOps to call gitlib.PushMultiple and gitlib.PullMultiple and validate PushResult/PullResult; then make verifyPushPull orchestrate these helpers (calling setupPushPullFixture, verifyPushFlow, verifyPullFlow, verifyMultipleOps) and return core.Result. Ensure you reference and update existing identifiers: verifyPushPull, setupPushPullFixture, verifyPushFlow, verifyPullFlow, verifyMultipleOps, commitFile, runGit, configureUser, gitlib.Push/ Pull/ PushMultiple/ PullMultiple, expectSingleStatus, cleanupTempDir.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@go/tests/cli/git/main.go`:
- Around line 313-314: The cmdArgs slice wrongly includes a leading "env" even
though Path is "/usr/bin/env", so change the creation of cmdArgs used in the
core.Cmd (the Args field) to start with "git" followed by args (e.g., build
cmdArgs from []string{"git"} + args) so the executed command becomes
"/usr/bin/env git ..." rather than "/usr/bin/env env git ..."; update the line
that sets cmdArgs and keep the core.Cmd{Path: "/usr/bin/env", Args: cmdArgs,
Dir: dir} intact.
---
Nitpick comments:
In `@go/service.go`:
- Line 72: The constant actionPathKey is defined using an odd string
concatenation ("p" + "ath"); either simplify it to a normal literal by replacing
actionPathKey = "p" + "ath" with actionPathKey = "path", or if the concatenation
is intentional (e.g., to avoid static analysis or pattern matching), add a brief
explanatory comment immediately above the declaration mentioning the reason and
linking to any relevant issue or test; update the declaration of actionPathKey
accordingly so the intent is clear to future readers.
- Line 145: Introduce a new constant (e.g., pathValidationFailed = "path
validation failed") alongside the other file-level constants and replace every
hard-coded "path validation failed" literal in this file (including the call
that currently passes it to c.LogError in the git handler:
c.LogError(resultError(paths), "git.handleQuery", "path validation failed"))
with that constant to remove duplication and improve maintainability; ensure all
five occurrences flagged by SonarCloud are updated to use the new constant.
In `@go/tests/cli/git/main.go`:
- Around line 243-262: Extract the repeated literal "relative/repo" into a named
constant (e.g., const relativeRepo = "relative/repo") near the top of the test
and replace the three usages in the block that call gitlib.Status(ctx,
gitlib.StatusOptions{Paths: []string{...}}), gitlib.Push(ctx, ...), and
gitlib.Pull(ctx, ...) with that constant; ensure references remain unchanged
(use the constant in StatusOptions.Paths slice and as the single argument to
Push and Pull) and run tests to verify no behavior change.
- Around line 131-217: The verifyPushPull function is too complex—split it into
smaller helpers: create setupPushPullFixture to create the temp repo, remote
(remote.git), pushClone and pullClone paths, run initial
init/clone/configureUser/initial commit and return the rootPath and clone paths
(and ensure defer cleanupTempDir is handled at top-level); implement
verifyPushFlow to perform local commit on pushClone, assert statuses via
expectSingleStatus and call gitlib.Push and re-check statuses; implement
verifyPullFlow to fetch on pullClone, assert statuses, call gitlib.Pull and
re-check statuses; implement verifyMultipleOps to call gitlib.PushMultiple and
gitlib.PullMultiple and validate PushResult/PullResult; then make verifyPushPull
orchestrate these helpers (calling setupPushPullFixture, verifyPushFlow,
verifyPullFlow, verifyMultipleOps) and return core.Result. Ensure you reference
and update existing identifiers: verifyPushPull, setupPushPullFixture,
verifyPushFlow, verifyPullFlow, verifyMultipleOps, commitFile, runGit,
configureUser, gitlib.Push/ Pull/ PushMultiple/ PullMultiple,
expectSingleStatus, cleanupTempDir.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6583aa67-b169-4764-9566-aa018dc9afe6
📒 Files selected for processing (5)
.golangci.bck.yml.golangci.ymlgo/git_test.gogo/service.gogo/tests/cli/git/main.go
✅ Files skipped from review due to trivial changes (1)
- .golangci.bck.yml
🚧 Files skipped from review as they are similar to previous changes (1)
- go/git_test.go
Comment on lines
+313
to
+314
| cmdArgs := append([]string{"env", "git"}, args...) | ||
| cmd := &core.Cmd{Path: "/usr/bin/env", Args: cmdArgs, Dir: dir} |
There was a problem hiding this comment.
Redundant "env" argument in command args.
The cmdArgs slice includes "env" as an argument, but /usr/bin/env is already the command path. The first argument to env should be git, not env git.
Proposed fix
func runGit(dir string, args ...string) core.Result {
- cmdArgs := append([]string{"env", "git"}, args...)
- cmd := &core.Cmd{Path: "/usr/bin/env", Args: cmdArgs, Dir: dir}
+ cmdArgs := append([]string{"git"}, args...)
+ cmd := &core.Cmd{Path: "/usr/bin/env", Args: cmdArgs, Dir: dir}📝 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.
Suggested change
| cmdArgs := append([]string{"env", "git"}, args...) | |
| cmd := &core.Cmd{Path: "/usr/bin/env", Args: cmdArgs, Dir: dir} | |
| cmdArgs := append([]string{"git"}, args...) | |
| cmd := &core.Cmd{Path: "/usr/bin/env", Args: cmdArgs, Dir: dir} |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@go/tests/cli/git/main.go` around lines 313 - 314, The cmdArgs slice wrongly
includes a leading "env" even though Path is "/usr/bin/env", so change the
creation of cmdArgs used in the core.Cmd (the Args field) to start with "git"
followed by args (e.g., build cmdArgs from []string{"git"} + args) so the
executed command becomes "/usr/bin/env git ..." rather than "/usr/bin/env env
git ..."; update the line that sets cmdArgs and keep the core.Cmd{Path:
"/usr/bin/env", Args: cmdArgs, Dir: dir} intact.
Mirrors api shape: .github/workflows/ci.yml runs test+coverage (Codecov), golangci-lint --tests=false, and sonarcloud-scan-action to dappcore_go-git. README gets the badge block (CI / quality gate / cov / security / maintainability / reliability / smells / NCLOC / pkg.go.dev / license). GOPROXY=direct GOSUMDB=off env in workflow to dodge the proxy.golang.org stale-zip pattern that broke api's first run. Internal Woodpecker pipeline at ci.lthn.sh continues unchanged.
Welcome to Codecov 🎉Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests. ℹ️ You can also turn on project coverage checks and project coverage reporting on Pull Request comment Thanks for integrating Codecov - We've got you covered ☂️ |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/ci.yml:
- Around line 73-80: The CI workflow is overriding sonar settings in
sonar-project.properties causing conflicting analysis scopes; either remove the
Sonar CLI args from the workflow (the -Dsonar.projectKey, -Dsonar.sources,
-Dsonar.tests, -Dsonar.test.inclusions, -Dsonar.go.coverage.reportPaths entries)
so the properties file becomes the single source of truth, or update the
properties file to match the values used in the workflow (e.g., set
sonar.projectKey to dappcore_go-git, sonar.sources to go, sonar.tests to go,
test inclusions and coverage path accordingly) so both places are consistent.
- Around line 15-16: The CI currently disables Go module integrity by setting
GOPROXY to "direct" and GOSUMDB to "off"; change these to enforce standard Go
security: restore GOPROXY to use the Go proxy (e.g., include
"https://proxy.golang.org" before "direct") and set GOSUMDB to the Go checksum
database (e.g., "sum.golang.org") so module downloads and checksums are
verified; update the workflow keys GOPROXY and GOSUMDB accordingly.
In `@README.md`:
- Around line 15-19: The README's Go Reference badge target (currently pointing
to dappco.re/go/go-git) does not match the declared module path
`dappco.re/go/git`; update the badge link and image target to use the correct
module path `dappco.re/go/git` so the pkg.go.dev badge points to the actual
package page (replace occurrences of `dappco.re/go/go-git` with
`dappco.re/go/git` in the badge markdown).
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1588379e-f085-4532-ba33-7b9a678545c5
📒 Files selected for processing (2)
.github/workflows/ci.ymlREADME.md
Comment on lines
+15
to
+16
| GOPROXY: "direct" | ||
| GOSUMDB: "off" |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Go modules, what are the security implications of setting GOSUMDB=off in CI, and what are the recommended GOPROXY/GOSUMDB settings for public dependencies?
💡 Result:
Setting GOSUMDB=off in CI disables the Go checksum database (sum.golang.org by default), which verifies that downloaded public modules match expected cryptographic hashes recorded in a transparent log. This check occurs when adding new entries to go.sum. Without it, the go command accepts any unrecognized public modules without verifying against the database, giving up the security guarantee of verified repeatable downloads and protection against tampered or malicious modules not yet in go.sum. This exposes CI builds to supply chain attacks where an attacker could serve malicious content for public dependencies, especially if go.sum is incomplete or new versions are fetched. Even with go.sum present, it only checks local cache matches; GOSUMDB provides an external trust anchor for public modules. For public dependencies, use the default settings: GOPROXY=https://proxy.golang.org,direct and GOSUMDB=sum.golang.org. These ensure modules are fetched reliably from the official proxy (with direct fallback) and verified against the checksum database. In CI, commit complete go.mod and go.sum files, use go build -mod=readonly to prevent modifications, and avoid disabling GOSUMDB. If using private modules, set GOPRIVATE and GONOSUMDB for those paths only, keeping public ones verified.
Citations:
- 1: https://goproxy.io/docs/GOSUMDB-env.html
- 2: https://github.com/golang/go/blob/master/go.env
- 3: https://go.dev/ref/mod
- 4: https://go.dev/design/25530-sumdb
- 5: Changes to default `GOSUMDB` environment variable on the Mend Developer Platform (and what it means for private Go modules) renovatebot/renovate#40041
- 6: https://tip.golang.org/blog/supply-chain
🏁 Script executed:
# Check if the workflow file exists
git ls-files '.github/workflows/ci.yml'Repository: dAppCore/go-git
Length of output: 84
🏁 Script executed:
# Read the CI workflow file to verify lines 15-16
cat -n .github/workflows/ci.yml | head -30Repository: dAppCore/go-git
Length of output: 768
🏁 Script executed:
# Check if there are any explanatory comments or documentation about these settings
grep -r "GOSUMDB\|GOPROXY" .github/ --include="*.yml" --include="*.yaml" -B 2 -A 2 || echo "No matches found"Repository: dAppCore/go-git
Length of output: 296
🏁 Script executed:
# Check if there's any go.sum or go.mod in the repository to understand the module setup
find . -name "go.sum" -o -name "go.mod" -type f | head -20Repository: dAppCore/go-git
Length of output: 83
🏁 Script executed:
# Check for any documentation or comments explaining the GOSUMDB/GOPROXY settings
find . -type f \( -name "README*" -o -name "*.md" \) -exec grep -l "GOSUMDB\|GOPROXY" {} \; 2>/dev/null | head -10Repository: dAppCore/go-git
Length of output: 41
🏁 Script executed:
# Check the go.mod file to understand the module configuration
cat -n ./go/go.mod | head -40Repository: dAppCore/go-git
Length of output: 158
🏁 Script executed:
# Search for any comments or issues related to these settings in the repository
git log --oneline --all --grep="GOSUMDB\|GOPROXY" 2>/dev/null | head -10 || echo "No git history found"Repository: dAppCore/go-git
Length of output: 128
🏁 Script executed:
# Check if there are any other workflows or setup steps that might affect module verification
git ls-files '.github/workflows/*.yml' '.github/workflows/*.yaml' 2>/dev/null | while read f; do echo "=== $f ==="; cat "$f" | grep -A 5 -B 5 "go.*download\|go.*mod\|GOSUMDB\|GOPROXY" || echo "No matches"; doneRepository: dAppCore/go-git
Length of output: 1048
Go module integrity checks are disabled in CI
Lines 15–16 disable both module proxy verification (GOPROXY: "direct") and checksum database verification (GOSUMDB: "off"). This removes protection against tampered or malicious module downloads and exposes the build to supply-chain attacks. Given the public dependency on dappco.re/go v0.9.0, checksum verification is essential.
Replace with standard Go security settings:
Suggested hardening
env:
GOFLAGS: -buildvcs=false
GOWORK: "off"
- GOPROXY: "direct"
- GOSUMDB: "off"
+ GOPROXY: "https://proxy.golang.org,direct"
+ GOSUMDB: "sum.golang.org"📝 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.
Suggested change
| GOPROXY: "direct" | |
| GOSUMDB: "off" | |
| env: | |
| GOFLAGS: -buildvcs=false | |
| GOWORK: "off" | |
| GOPROXY: "https://proxy.golang.org,direct" | |
| GOSUMDB: "sum.golang.org" |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/ci.yml around lines 15 - 16, The CI currently disables Go
module integrity by setting GOPROXY to "direct" and GOSUMDB to "off"; change
these to enforce standard Go security: restore GOPROXY to use the Go proxy
(e.g., include "https://proxy.golang.org" before "direct") and set GOSUMDB to
the Go checksum database (e.g., "sum.golang.org") so module downloads and
checksums are verified; update the workflow keys GOPROXY and GOSUMDB
accordingly.
Comment on lines
+73
to
+80
| args: > | ||
| -Dsonar.organization=dappcore | ||
| -Dsonar.projectKey=dappcore_go-git | ||
| -Dsonar.sources=go | ||
| -Dsonar.exclusions=**/vendor/**,**/third_party/**,**/.tmp/**,**/*_test.go | ||
| -Dsonar.tests=go | ||
| -Dsonar.test.inclusions=**/*_test.go | ||
| -Dsonar.go.coverage.reportPaths=go/coverage.out |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "sonar\\.projectKey|sonar\\.sources|sonar\\.tests|sonar\\.go\\.coverage\\.reportPaths" sonar-project.properties .github/workflows/ci.ymlRepository: dAppCore/go-git
Length of output: 551
🏁 Script executed:
cat -n sonar-project.propertiesRepository: dAppCore/go-git
Length of output: 643
🌐 Web query:
SonarQube command-line properties override sonar-project.properties precedence
💡 Result:
Yes, SonarQube command-line properties override those in sonar-project.properties. The official documentation states the settings hierarchy in order of precedence: 1. Global properties (UI), 2. Project properties (UI), 3. Scanner configuration files (like sonar-project.properties), 4. Scanner arguments (command-line -D parameters). Command-line arguments explicitly override scanner configuration files. This applies to the SonarScanner CLI (sonar-scanner command). Example: sonar-scanner -Dsonar.projectKey=myproject -Dsonar.sources=src1 will override values from sonar-project.properties.
Citations:
- 1: https://docs.sonarsource.com/sonarqube-server/analyzing-source-code/analysis-parameters/configuration-overview
- 2: https://docs.sonarsource.com/sonarqube-server/10.8/analyzing-source-code/analysis-parameters
- 3: https://docs.sonarsource.com/sonarqube-server/latest/analyzing-source-code/scanners/sonarscanner
- 4: https://docs.sonarsource.com/sonarqube-server/2025.2/analyzing-source-code/analysis-parameters
Conflicting SonarCloud configuration between workflow and sonar-project.properties
Workflow arguments set sonar.projectKey=dappcore_go-git, sonar.sources=go, and sonar.tests=go, while sonar-project.properties defines different values (core_go-git, .). These will produce different analysis scopes, with the workflow arguments taking precedence. Consolidate to a single source of truth by either removing the conflicting workflow arguments and relying on the properties file, or updating the properties file to match the workflow configuration.
Suggested simplification (remove conflicting workflow args)
- name: SonarCloud Scan
uses: SonarSource/sonarqube-scan-action@v6
env:
SONAR_TOKEN: ${{ secrets.SONAR_TOKEN }}
with:
args: >
-Dsonar.organization=dappcore
- -Dsonar.projectKey=dappcore_go-git
- -Dsonar.sources=go
- -Dsonar.exclusions=**/vendor/**,**/third_party/**,**/.tmp/**,**/*_test.go
- -Dsonar.tests=go
- -Dsonar.test.inclusions=**/*_test.go
-Dsonar.go.coverage.reportPaths=go/coverage.out📝 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.
Suggested change
| args: > | |
| -Dsonar.organization=dappcore | |
| -Dsonar.projectKey=dappcore_go-git | |
| -Dsonar.sources=go | |
| -Dsonar.exclusions=**/vendor/**,**/third_party/**,**/.tmp/**,**/*_test.go | |
| -Dsonar.tests=go | |
| -Dsonar.test.inclusions=**/*_test.go | |
| -Dsonar.go.coverage.reportPaths=go/coverage.out | |
| args: > | |
| -Dsonar.organization=dappcore | |
| -Dsonar.go.coverage.reportPaths=go/coverage.out |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/ci.yml around lines 73 - 80, The CI workflow is overriding
sonar settings in sonar-project.properties causing conflicting analysis scopes;
either remove the Sonar CLI args from the workflow (the -Dsonar.projectKey,
-Dsonar.sources, -Dsonar.tests, -Dsonar.test.inclusions,
-Dsonar.go.coverage.reportPaths entries) so the properties file becomes the
single source of truth, or update the properties file to match the values used
in the workflow (e.g., set sonar.projectKey to dappcore_go-git, sonar.sources to
go, sonar.tests to go, test inclusions and coverage path accordingly) so both
places are consistent.
Comment on lines
+15
to
19
| [](https://pkg.go.dev/dappco.re/go/go-git) | ||
| [](https://eupl.eu/1.2/en/) | ||
|
|
||
| # go-git | ||
|
|
||
| Go module: `dappco.re/go/git` |
There was a problem hiding this comment.
Go Reference badge target does not match the declared module path
Line 15 points to dappco.re/go/go-git, but Line 19 declares dappco.re/go/git. This likely leaves the badge pointing at the wrong package page.
Suggested fix
-[](https://pkg.go.dev/dappco.re/go/go-git)
+[](https://pkg.go.dev/dappco.re/go/git)📝 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.
Suggested change
| [](https://pkg.go.dev/dappco.re/go/go-git) | |
| [](https://eupl.eu/1.2/en/) | |
| # go-git | |
| Go module: `dappco.re/go/git` | |
| [](https://pkg.go.dev/dappco.re/go/git) | |
| [](https://eupl.eu/1.2/en/) | |
| Go module: `dappco.re/go/git` |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@README.md` around lines 15 - 19, The README's Go Reference badge target
(currently pointing to dappco.re/go/go-git) does not match the declared module
path `dappco.re/go/git`; update the badge link and image target to use the
correct module path `dappco.re/go/git` so the pkg.go.dev badge points to the
actual package page (replace occurrences of `dappco.re/go/go-git` with
`dappco.re/go/git` in the badge markdown).
Per-rule: - S1192 (string-literal dup → const): 12 - S3776 (cognitive complexity): 1 - yaml:DocumentStartCheck: 1 vet/test/golangci-lint clean. No exported API changed.
|
|
There was a problem hiding this comment.
♻️ Duplicate comments (3)
go/service_test.go (2)
32-41:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
TestService_NewService_Badcurrently asserts the opposite of the path policy.Line 38 should assert failure for a relative
WorkDir; otherwise this test locks in non-compliant behaviour.Suggested fix
func TestService_NewService_Bad(t *T) { c := New() opts := ServiceOptions{WorkDir: relativeWorkDir} r := NewService(opts)(c) - AssertTrue(t, r.OK) - svc := r.Value.(*Service) - AssertEqual(t, relativeWorkDir, svc.opts.WorkDir) + AssertFalse(t, r.OK) + AssertContains(t, r.Error(), pathMustBeAbsolute) + AssertNil(t, r.Value) }As per coding guidelines:
All file paths must be absolute; Service enforces WorkDir boundary via validatePath.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@go/service_test.go` around lines 32 - 41, The test TestService_NewService_Bad currently asserts success for a relative WorkDir; change it to assert failure so it enforces the absolute-path policy: when calling NewService with ServiceOptions{WorkDir: relativeWorkDir}, the result from NewService(opts)(c) should be a failure (use AssertFalse or the test helper that checks r.OK is false) and optionally verify the error/path validation via Service.validatePath or that svc is nil; update references to NewService, ServiceOptions, Service and validatePath accordingly so the test fails for relative paths.
16-18: 🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy lift
testServicefixture bypasses required real-repo setup for service-path tests.Using synthetic
lastStatusfixtures as the base pattern here prevents exercising real git state transitions expected by the test policy.As per coding guidelines:
go/**/*_test.go: Create real git repos in temp directories using initTestRepo() for test fixtures.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@go/service_test.go` around lines 16 - 18, The testService fixture currently constructs a Service with synthetic lastStatus values (function testService and field lastStatus) which bypasses required real-repo setup; update the fixture so it creates a real temporary git repo via initTestRepo() and initializes the Service against that repo (or add an overloaded test helper that accepts the repo path), then drive status changes by manipulating the real repo rather than directly setting lastStatus; locate and modify testService in service_test.go and any tests using it to use the initTestRepo-backed helper so tests exercise actual git state transitions.go/tests/cli/git/main.go (1)
343-345:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
runGitprepends an extraenvargument.Line 344 currently builds
/usr/bin/env env git ...; this should be/usr/bin/env git ....Suggested fix
func runGit(dir string, args ...string) core.Result { - cmdArgs := append([]string{"env", "git"}, args...) + cmdArgs := append([]string{"git"}, args...) cmd := &core.Cmd{Path: "/usr/bin/env", Args: cmdArgs, Dir: dir}#!/bin/bash rg -n 'cmdArgs := append\(\[\]string\{"env", "git"\}, args\.\.\.\)' go/tests/cli/git/main.go🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@go/tests/cli/git/main.go` around lines 343 - 345, The runGit function builds the command args with an extra "env" so it currently yields "/usr/bin/env env git ..."; change the cmdArgs construction in runGit to not prepend the extra "env" (e.g. build cmdArgs with "git" and the passed args) while keeping Path: "/usr/bin/env" and Dir set on the core.Cmd; update the line that creates cmdArgs (and any similar occurrences) to use append([]string{"git"}, args...) so the executed command becomes "/usr/bin/env git ...".
🧹 Nitpick comments (1)
go/git_test.go (1)
143-447: ⚡ Quick win
_Uglytest suffixes violate the required naming pattern.Please rename these to the approved
_Good/_Badconvention to keep the suite policy-compliant.As per coding guidelines:
go/**/*_test.go: Use _Good / _Bad suffix pattern for test function names indicating success / failure cases.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@go/git_test.go` around lines 143 - 447, Several tests use the forbidden "_Ugly" suffix; rename those test functions to use the approved _Good/_Bad pattern by replacing the "_Ugly" suffix with "_Bad". Specifically update TestGit_RepoStatus_IsDirty_Ugly, TestGit_RepoStatus_HasUnpushed_Ugly, TestGit_RepoStatus_HasUnpulled_Ugly, TestGit_Status_Ugly, TestGit_StatusIter_Ugly, TestGit_Push_Ugly, TestGit_Pull_Ugly, TestGit_IsNonFastForward_Ugly, TestGit_PushMultiple_Ugly, TestGit_PushMultipleIter_Ugly, TestGit_PullMultiple_Ugly, TestGit_PullMultipleIter_Ugly, and TestGit_GitError_Error_Ugly to respectively end with "_Bad" (e.g., TestGit_RepoStatus_IsDirty_Bad) so the test names comply with the go/**/*_test.go naming convention.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@go/service_test.go`:
- Around line 32-41: The test TestService_NewService_Bad currently asserts
success for a relative WorkDir; change it to assert failure so it enforces the
absolute-path policy: when calling NewService with ServiceOptions{WorkDir:
relativeWorkDir}, the result from NewService(opts)(c) should be a failure (use
AssertFalse or the test helper that checks r.OK is false) and optionally verify
the error/path validation via Service.validatePath or that svc is nil; update
references to NewService, ServiceOptions, Service and validatePath accordingly
so the test fails for relative paths.
- Around line 16-18: The testService fixture currently constructs a Service with
synthetic lastStatus values (function testService and field lastStatus) which
bypasses required real-repo setup; update the fixture so it creates a real
temporary git repo via initTestRepo() and initializes the Service against that
repo (or add an overloaded test helper that accepts the repo path), then drive
status changes by manipulating the real repo rather than directly setting
lastStatus; locate and modify testService in service_test.go and any tests using
it to use the initTestRepo-backed helper so tests exercise actual git state
transitions.
In `@go/tests/cli/git/main.go`:
- Around line 343-345: The runGit function builds the command args with an extra
"env" so it currently yields "/usr/bin/env env git ..."; change the cmdArgs
construction in runGit to not prepend the extra "env" (e.g. build cmdArgs with
"git" and the passed args) while keeping Path: "/usr/bin/env" and Dir set on the
core.Cmd; update the line that creates cmdArgs (and any similar occurrences) to
use append([]string{"git"}, args...) so the executed command becomes
"/usr/bin/env git ...".
---
Nitpick comments:
In `@go/git_test.go`:
- Around line 143-447: Several tests use the forbidden "_Ugly" suffix; rename
those test functions to use the approved _Good/_Bad pattern by replacing the
"_Ugly" suffix with "_Bad". Specifically update TestGit_RepoStatus_IsDirty_Ugly,
TestGit_RepoStatus_HasUnpushed_Ugly, TestGit_RepoStatus_HasUnpulled_Ugly,
TestGit_Status_Ugly, TestGit_StatusIter_Ugly, TestGit_Push_Ugly,
TestGit_Pull_Ugly, TestGit_IsNonFastForward_Ugly, TestGit_PushMultiple_Ugly,
TestGit_PushMultipleIter_Ugly, TestGit_PullMultiple_Ugly,
TestGit_PullMultipleIter_Ugly, and TestGit_GitError_Error_Ugly to respectively
end with "_Bad" (e.g., TestGit_RepoStatus_IsDirty_Bad) so the test names comply
with the go/**/*_test.go naming convention.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ceea63b3-870a-4ac6-810a-1c4007a04008
📒 Files selected for processing (6)
go/git_example_test.gogo/git_test.gogo/service.gogo/service_test.gogo/tests/cli/git/Taskfile.yamlgo/tests/cli/git/main.go
✅ Files skipped from review due to trivial changes (3)
- go/tests/cli/git/Taskfile.yaml
- go/git_example_test.go
- go/service.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.
Brings this repo to verdict: COMPLIANT against the v0.9.0 audit.
🤖 Generated with Claude Code + Codex
Co-Authored-By: Codex noreply@openai.com
Co-Authored-By: Virgil virgil@lethean.io
Summary by CodeRabbit
Chores
Tests
Documentation