Conversation
Migrate forge.lthn.ai/core/go-log to dappco.re/go/core/log v0.1.0 and bump dappco.re/go/core from v0.4.7 to v0.5.0. Remove coreerr dependency from git.go (stdlib-only by design). Extract filterStatuses helper in service.go to deduplicate Dirty/Ahead iterators. Co-Authored-By: Virgil <virgil@lethean.io> Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Virgil <virgil@lethean.io>
Migrate from RegisterTask to named Actions (git.push, git.pull, git.push-multiple). OnStartup now returns core.Result. Replace banned stdlib imports with Core primitives. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Virgil <virgil@lethean.io>
Co-Authored-By: Virgil <virgil@lethean.io>
Co-Authored-By: Virgil <virgil@lethean.io>
Co-Authored-By: Virgil <virgil@lethean.io>
Co-Authored-By: Virgil <virgil@lethean.io>
Co-Authored-By: Virgil <virgil@lethean.io>
Co-Authored-By: Virgil <virgil@lethean.io>
Co-Authored-By: Virgil <virgil@lethean.io>
Co-Authored-By: Virgil <virgil@lethean.io>
Co-Authored-By: Virgil <virgil@lethean.io>
Co-Authored-By: Virgil <virgil@lethean.io>
Co-Authored-By: Virgil <virgil@lethean.io>
Co-Authored-By: Virgil <virgil@lethean.io>
Co-Authored-By: Virgil <virgil@lethean.io>
Co-Authored-By: Virgil <virgil@lethean.io>
Co-Authored-By: Virgil <virgil@lethean.io>
Co-Authored-By: Virgil <virgil@lethean.io>
Co-Authored-By: Virgil <virgil@lethean.io>
📝 WalkthroughWalkthroughAdds iterator-based APIs for status and multi-repo operations, introduces PullResult and behind-repo queries/tasks, refactors service startup to register Core actions, centralises path validation, updates module path/dependencies, and expands tests and documentation. Changes
Sequence Diagram(s)mermaid 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches⚔️ Resolve merge conflicts
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.11.4)Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions Comment |
Co-Authored-By: Virgil <virgil@lethean.io>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (5)
service_test.go (1)
200-208: These two tests don't exercise behaviour.If the package compiles, both
IsTypeassertions are guaranteed. I'd drop them or replace them with a real query/task behaviour test, and rename it with the_Good/_Badsuffix convention. As per coding guidelines,service_test.go: "Service helper tests must be inservice_test.goand constructServicestructs directly without the framework"; and**/*_test.go: "Test files must use_Good/_Badsuffix pattern for success / failure test cases".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@service_test.go` around lines 200 - 208, The two tests only assert types and should be replaced or removed: delete TestQueryBehindRepos_TypeExists and TestTaskPullMultiple_TypeExists and instead add meaningful behavior tests that construct Service structs directly (per service_test.go guideline) and exercise QueryBehindRepos and TaskPullMultiple logic (e.g., success and failure flows), naming them with the _Good/_Bad suffixes (e.g., TestQueryBehindRepos_Good / TestQueryBehindRepos_Bad and TestTaskPullMultiple_Good / TestTaskPullMultiple_Bad) so they validate real behavior rather than just compile-time types.service_extra_test.go (4)
84-89: Remove unnecessary_ = svcstatements.The
svcvariable is already used when callingsvc.OnStartup(...). The blank identifier assignments on lines 89, 105, 122, and 139 are redundant and can be removed.Proposed cleanup
result := c.Action("git.push").Run(context.Background(), core.NewOptions( core.Option{Key: "path", Value: "relative/path"}, )) - _ = svc assert.False(t, result.OK)Apply similar removal to lines 105, 122, and 139.
Also applies to: 100-105, 116-123, 133-140
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@service_extra_test.go` around lines 84 - 89, Remove the redundant blank-identifier assignments of svc (e.g., `_ = svc`) that follow svc.OnStartup(...) and subsequent test actions; locate occurrences of `_ = svc` tied to the svc variable in this file (they appear after calls like svc.OnStartup(...) and after c.Action(...).Run(...)) and delete those lines so svc is only referenced where actually used.
438-487: Test naming may cause confusion.Tests named
_Good_PushMultipleand_Good_PullMultipleassertresult.OK == false. While technically testing "good behaviour" (structured results are returned), the_Goodsuffix typically indicates expected success. Consider renaming to_Bad_PushMultipleNoRemoteor similar to match the_Good/_Badnaming convention.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@service_extra_test.go` around lines 438 - 487, Rename the misleading test functions TestService_Action_Good_PushMultiple and TestService_Action_Good_PullMultiple to names that reflect the expected failure outcome (e.g., TestService_Action_Bad_PushMultiple_NoRemote and TestService_Action_Bad_PullMultiple_NoRemote), update any references to these function names in the file, and keep the test bodies identical so the assertions (result.OK == false and checks on PushResult/PullResult) clearly match the new names.
489-533: Same naming concern as above.
TestService_HandleTask_Good_PushMultipleandTestService_HandleTask_Good_PullMultipleboth assertresult.OK == false. Consider aligning with the_Badsuffix convention when the operation is expected to fail.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@service_extra_test.go` around lines 489 - 533, Tests TestService_HandleTask_Good_PushMultiple and TestService_HandleTask_Good_PullMultiple are named with the "_Good_" prefix but assert failures (result.OK == false); rename them to use the "_Bad_" suffix to match the convention and update any references: change function names TestService_HandleTask_Good_PushMultiple -> TestService_HandleTask_Bad_PushMultiple and TestService_HandleTask_Good_PullMultiple -> TestService_HandleTask_Bad_PullMultiple, and if there are any test helpers or calls referencing the old names update those references as well to keep naming consistent with expected failure assertions.
317-320: Type assertions without comma-ok can panic.If
result.Valueis unexpectedly not anerror, the direct type assertion will panic, causing a confusing test failure. Using testify'srequire.IsTypeor comma-ok pattern is safer.Proposed fix
result := svc.handleTaskMessage(c, struct{}{}) assert.False(t, result.OK) - assert.Error(t, result.Value.(error)) - assert.Contains(t, result.Value.(error).Error(), "unsupported task message type") + err, ok := result.Value.(error) + require.True(t, ok, "expected result.Value to be an error") + assert.Contains(t, err.Error(), "unsupported task message type")Apply similar pattern to lines 332-333.
Also applies to: 330-333
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@service_extra_test.go` around lines 317 - 320, The test uses direct type assertions on result.Value which can panic if Value isn't an error; update the assertions around svc.handleTaskMessage and the other occurrences (lines ~330-333) to first assert the type safely—e.g., use require.IsType(t, (*error)(nil), result.Value) or use the comma-ok pattern to cast: val, ok := result.Value.(error); require.True(t, ok) and then assert on val (assert.Error(t, val) and assert.Contains(t, val.Error(), "unsupported task message type")) so the test fails cleanly instead of panicking.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/architecture.md`:
- Line 162: The architecture doc is inconsistent: update the lifecycle snippet
to match current service.go signatures and APIs by replacing the old
OnStartup(ctx) error and RegisterTask references with the new OnStartup(ctx)
core.Result and RegisterAction usage and show named actions; ensure examples
reference QueryBehindRepos and TaskPullMultiple where relevant and adjust any
lifecycle flow text to use RegisterAction instead of RegisterTask to match
service.go.
In `@docs/index.md`:
- Line 92: The docs still reference the retired namespace "forge.lthn.ai" in the
sample imports/dependency; update every example/import/dependency that uses
"forge.lthn.ai/..." to "dappco.re/..." (including the quick-start examples
around the fmt.Printf example and the surrounding lines that show module/import
declarations), so the sample code (symbols like fmt.Printf and variables such as
s.Name, s.IsDirty(), s.HasUnpushed(), s.HasUnpulled()) and any dependency name
in the example module become copy-pasteable with the new dappco.re namespace.
In `@git_test.go`:
- Around line 270-283: Replace the generic assert.Error checks with assertions
that the returned error is a *GitError and contains the expected operation and
captured args/stderr: use core.As(err, new(*GitError)) (or core.As(&gerr)) to
cast and then assert gerr.Op equals the operation name for gitCommand/Push/Pull
and that gerr.Args includes the provided relative path and gerr.Stderr is
non-empty; update TestGitCommand_Bad_RelativePath, TestPush_Bad_RelativePath,
and TestPull_Bad_RelativePath accordingly so they validate the error shape (Op,
Args, Stderr) instead of only assert.Error.
- Around line 742-770: The two tests
TestGetStatus_Good_TypeChangedFile_WorkingTree and
TestGetStatus_Good_TypeChangedFile_Staged hard-code "/etc/hosts" and
unconditionally call os.Symlink making them fail on Windows; replace the
hard-coded target with a file created in a t.TempDir (or os.TempDir) and attempt
to create the symlink to that temp file, and if os.Symlink returns an error
indicating symlinks are unsupported or permission denied, call t.Skipf to skip
the test; ensure the creation and removal steps around core.JoinPath(dir,
"README.md") remain the same and keep asserting on status.Modified/status.Staged
and status.IsDirty() after the symlink operation.
In `@git.go`:
- Around line 221-224: The strconv.Atoi parse failures (e.g., when parsing
aheadStr/behindStr around the ahead/behind logic using trim and strconv.Atoi)
must not be returned as plain fmt.Errorf; instead wrap them into a *GitError
that includes the original parse error, the captured stderr and the command
arguments. Replace the current returns that do fmt.Errorf("failed to parse
ahead/behind count: %w", err) with construction of a *GitError (or the existing
helper that builds GitError) embedding the parse error as the inner error and
setting Stderr and Args from the command context so both the stderr and args are
preserved; apply the same change to the similar parse handling around lines
235-238.
In `@llms.txt`:
- Line 3: Update the module declaration in llms.txt by replacing the retired
module path string "forge.lthn.ai/core/go-git" with the new namespace
"dappco.re/go/core/git"; locate the module line that currently contains
forge.lthn.ai/core/go-git and change it to the exact new path so machine
consumers resolve to the migrated package.
In `@README.md`:
- Around line 9-10: The README still references the old module path
"forge.lthn.ai/core/go-git" in the badge and module declaration; update both
occurrences to the new module path "dappco.re/go/core/git" and ensure the Agent
index reference to `llms.txt` remains intact (check the badge image URL and any
"module" or "go get" line in README and replace the host/path to
`dappco.re/go/core/git` so all references are consistent with the migration).
In `@service.go`:
- Around line 240-247: The validatePath() WorkDir validation currently returns
coreerr.E(...) which breaks the package contract that all service errors must be
*GitError; change both failure branches (non-absolute WorkDir and path outside
WorkDir) to return a *GitError instead by wrapping the core error using the
package's GitError constructor/helper used elsewhere (search for
NewGitError/MakeGitError or a wrapGitError helper) so the returned error
preserves Args and Stderr fields consistent with other git errors from this
package (keep references to validatePath, WorkDir, and the coreerr.E message
text when wrapping).
---
Nitpick comments:
In `@service_extra_test.go`:
- Around line 84-89: Remove the redundant blank-identifier assignments of svc
(e.g., `_ = svc`) that follow svc.OnStartup(...) and subsequent test actions;
locate occurrences of `_ = svc` tied to the svc variable in this file (they
appear after calls like svc.OnStartup(...) and after c.Action(...).Run(...)) and
delete those lines so svc is only referenced where actually used.
- Around line 438-487: Rename the misleading test functions
TestService_Action_Good_PushMultiple and TestService_Action_Good_PullMultiple to
names that reflect the expected failure outcome (e.g.,
TestService_Action_Bad_PushMultiple_NoRemote and
TestService_Action_Bad_PullMultiple_NoRemote), update any references to these
function names in the file, and keep the test bodies identical so the assertions
(result.OK == false and checks on PushResult/PullResult) clearly match the new
names.
- Around line 489-533: Tests TestService_HandleTask_Good_PushMultiple and
TestService_HandleTask_Good_PullMultiple are named with the "_Good_" prefix but
assert failures (result.OK == false); rename them to use the "_Bad_" suffix to
match the convention and update any references: change function names
TestService_HandleTask_Good_PushMultiple ->
TestService_HandleTask_Bad_PushMultiple and
TestService_HandleTask_Good_PullMultiple ->
TestService_HandleTask_Bad_PullMultiple, and if there are any test helpers or
calls referencing the old names update those references as well to keep naming
consistent with expected failure assertions.
- Around line 317-320: The test uses direct type assertions on result.Value
which can panic if Value isn't an error; update the assertions around
svc.handleTaskMessage and the other occurrences (lines ~330-333) to first assert
the type safely—e.g., use require.IsType(t, (*error)(nil), result.Value) or use
the comma-ok pattern to cast: val, ok := result.Value.(error); require.True(t,
ok) and then assert on val (assert.Error(t, val) and assert.Contains(t,
val.Error(), "unsupported task message type")) so the test fails cleanly instead
of panicking.
In `@service_test.go`:
- Around line 200-208: The two tests only assert types and should be replaced or
removed: delete TestQueryBehindRepos_TypeExists and
TestTaskPullMultiple_TypeExists and instead add meaningful behavior tests that
construct Service structs directly (per service_test.go guideline) and exercise
QueryBehindRepos and TaskPullMultiple logic (e.g., success and failure flows),
naming them with the _Good/_Bad suffixes (e.g., TestQueryBehindRepos_Good /
TestQueryBehindRepos_Bad and TestTaskPullMultiple_Good /
TestTaskPullMultiple_Bad) so they validate real behavior rather than just
compile-time types.
🪄 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: 8e986fda-c8d9-4b9d-b831-a3a237b8e56f
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (10)
README.mddocs/architecture.mddocs/index.mdgit.gogit_test.gogo.modllms.txtservice.goservice_extra_test.goservice_test.go
Comment on lines
+270
to
+283
| func TestGitCommand_Bad_RelativePath(t *testing.T) { | ||
| _, err := gitCommand(context.Background(), "relative/path", "status") | ||
| assert.Error(t, err) | ||
| } | ||
|
|
||
| func TestPush_Bad_RelativePath(t *testing.T) { | ||
| err := Push(context.Background(), "relative/path") | ||
| assert.Error(t, err) | ||
| } | ||
|
|
||
| func TestPull_Bad_RelativePath(t *testing.T) { | ||
| err := Pull(context.Background(), "relative/path") | ||
| assert.Error(t, err) | ||
| } |
There was a problem hiding this comment.
Assert the *GitError shape here, not just assert.Error.
These cases only prove that something failed. They won't catch a regression where path validation stops populating Args/Stderr. Please assert core.As(..., *GitError) and the expected op name on the single- and multi-repo paths. As per coding guidelines, **/*.go: "All errors must be wrapped as *GitError with captured stderr and command args".
Also applies to: 609-639
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@git_test.go` around lines 270 - 283, Replace the generic assert.Error checks
with assertions that the returned error is a *GitError and contains the expected
operation and captured args/stderr: use core.As(err, new(*GitError)) (or
core.As(&gerr)) to cast and then assert gerr.Op equals the operation name for
gitCommand/Push/Pull and that gerr.Args includes the provided relative path and
gerr.Stderr is non-empty; update TestGitCommand_Bad_RelativePath,
TestPush_Bad_RelativePath, and TestPull_Bad_RelativePath accordingly so they
validate the error shape (Op, Args, Stderr) instead of only assert.Error.
Comment on lines
+742
to
+770
| func TestGetStatus_Good_TypeChangedFile_WorkingTree(t *testing.T) { | ||
| dir, _ := filepath.Abs(initTestRepo(t)) | ||
|
|
||
| // Replace the tracked file with a symlink to trigger a working-tree type change. | ||
| require.NoError(t, os.Remove(core.JoinPath(dir, "README.md"))) | ||
| require.NoError(t, os.Symlink("/etc/hosts", core.JoinPath(dir, "README.md"))) | ||
|
|
||
| status := getStatus(context.Background(), dir, "typechanged-working-tree") | ||
| require.NoError(t, status.Error) | ||
| assert.Equal(t, 1, status.Modified, "type change in working tree counts as modified") | ||
| assert.True(t, status.IsDirty()) | ||
| } | ||
|
|
||
| func TestGetStatus_Good_TypeChangedFile_Staged(t *testing.T) { | ||
| dir, _ := filepath.Abs(initTestRepo(t)) | ||
|
|
||
| // Stage a type change by replacing the tracked file with a symlink and adding it. | ||
| require.NoError(t, os.Remove(core.JoinPath(dir, "README.md"))) | ||
| require.NoError(t, os.Symlink("/etc/hosts", core.JoinPath(dir, "README.md"))) | ||
|
|
||
| cmd := exec.Command("git", "add", "README.md") | ||
| cmd.Dir = dir | ||
| require.NoError(t, cmd.Run()) | ||
|
|
||
| status := getStatus(context.Background(), dir, "typechanged-staged") | ||
| require.NoError(t, status.Error) | ||
| assert.Equal(t, 1, status.Staged, "type change in the index counts as staged") | ||
| assert.True(t, status.IsDirty()) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'Symlink-based tests in git_test.go:\n'
sed -n '742,770p' git_test.go
printf '\nDeclared build targets (if present):\n'
if [ -f .core/build.yaml ]; then
sed -n '1,200p' .core/build.yaml
else
printf '.core/build.yaml not found\n'
fiRepository: dAppCore/go-git
Length of output: 1638
These type-change tests will fail on Windows runners.
Both tests hard-code /etc/hosts and unconditionally call os.Symlink. The repo declares Windows as a build target in .core/build.yaml, so these cases are not portable. Additionally, symlink creation on Windows often requires elevated privileges or Developer Mode. Replace the hard-coded path with a temp-directory target and use t.Skip to skip the tests when symlink creation is unavailable.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@git_test.go` around lines 742 - 770, The two tests
TestGetStatus_Good_TypeChangedFile_WorkingTree and
TestGetStatus_Good_TypeChangedFile_Staged hard-code "/etc/hosts" and
unconditionally call os.Symlink making them fail on Windows; replace the
hard-coded target with a file created in a t.TempDir (or os.TempDir) and attempt
to create the symlink to that temp file, and if os.Symlink returns an error
indicating symlinks are unsupported or permission denied, call t.Skipf to skip
the test; ensure the creation and removal steps around core.JoinPath(dir,
"README.md") remain the same and keep asserting on status.Modified/status.Staged
and status.IsDirty() after the symlink operation.
Comment on lines
+221
to
224
| ahead, err = strconv.Atoi(trim(aheadStr)) | ||
| if err != nil { | ||
| return 0, 0, fmt.Errorf("failed to parse ahead count: %w", err) | ||
| } |
There was a problem hiding this comment.
Parse errors should be wrapped as *GitError.
Per coding guidelines, all errors must be wrapped as *GitError with captured stderr and command args. The strconv.Atoi parse failures are returned as plain fmt.Errorf.
Proposed fix
if err == nil {
ahead, err = strconv.Atoi(trim(aheadStr))
if err != nil {
- return 0, 0, fmt.Errorf("failed to parse ahead count: %w", err)
+ return 0, 0, &GitError{
+ Args: []string{"rev-list", "--count", "@{u}..HEAD"},
+ Err: fmt.Errorf("failed to parse ahead count: %w", err),
+ Stderr: "",
+ }
}
} else if isNoUpstreamError(err) { if err == nil {
behind, err = strconv.Atoi(trim(behindStr))
if err != nil {
- return 0, 0, fmt.Errorf("failed to parse behind count: %w", err)
+ return 0, 0, &GitError{
+ Args: []string{"rev-list", "--count", "HEAD..@{u}"},
+ Err: fmt.Errorf("failed to parse behind count: %w", err),
+ Stderr: "",
+ }
}
} else if isNoUpstreamError(err) {As per coding guidelines: "All errors must be wrapped as *GitError with captured stderr and command args".
Also applies to: 235-238
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@git.go` around lines 221 - 224, The strconv.Atoi parse failures (e.g., when
parsing aheadStr/behindStr around the ahead/behind logic using trim and
strconv.Atoi) must not be returned as plain fmt.Errorf; instead wrap them into a
*GitError that includes the original parse error, the captured stderr and the
command arguments. Replace the current returns that do fmt.Errorf("failed to
parse ahead/behind count: %w", err) with construction of a *GitError (or the
existing helper that builds GitError) embedding the parse error as the inner
error and setting Stderr and Args from the command context so both the stderr
and args are preserved; apply the same change to the similar parse handling
around lines 235-238.
| @@ -0,0 +1,38 @@ | |||
| # go-git | |||
|
|
|||
| Module: `forge.lthn.ai/core/go-git` | |||
There was a problem hiding this comment.
llms.txt points agents at the retired module path.
Line 3 still declares forge.lthn.ai/core/go-git, which undermines the namespace migration and will misdirect machine consumers. Please switch it to dappco.re/go/core/git.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@llms.txt` at line 3, Update the module declaration in llms.txt by replacing
the retired module path string "forge.lthn.ai/core/go-git" with the new
namespace "dappco.re/go/core/git"; locate the module line that currently
contains forge.lthn.ai/core/go-git and change it to the exact new path so
machine consumers resolve to the migrated package.
Comment on lines
+9
to
+10
| Agent index: [`llms.txt`](llms.txt) | ||
|
|
There was a problem hiding this comment.
README still advertises the old module path.
Adding the agent index is helpful, but the badge and module line above still point to forge.lthn.ai/core/go-git. Please update those to dappco.re/go/core/git as part of the same migration.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@README.md` around lines 9 - 10, The README still references the old module
path "forge.lthn.ai/core/go-git" in the badge and module declaration; update
both occurrences to the new module path "dappco.re/go/core/git" and ensure the
Agent index reference to `llms.txt` remains intact (check the badge image URL
and any "module" or "go get" line in README and replace the host/path to
`dappco.re/go/core/git` so all references are consistent with the migration).
Comment on lines
+240
to
+247
| workDir = filepath.Clean(workDir) | ||
| if !filepath.IsAbs(workDir) { | ||
| return coreerr.E("git.validatePath", "WorkDir must be absolute: "+s.opts.WorkDir, nil) | ||
| } | ||
| rel, err := filepath.Rel(workDir, filepath.Clean(path)) | ||
| if err != nil || rel == ".." || core.HasPrefix(rel, ".."+string(filepath.Separator)) { | ||
| return coreerr.E("git.validatePath", "path "+path+" is outside of allowed WorkDir "+workDir, nil) | ||
| } |
There was a problem hiding this comment.
Preserve the package's *GitError contract in validatePath().
These new WorkDir validation branches return coreerr.E(...), so service-level path failures no longer look like the *GitError values produced elsewhere in the package. That makes Args/Stderr inspection inconsistent for callers using the service API. Please wrap these validation failures as *GitError too, or reuse a shared helper that does so. As per coding guidelines, **/*.go: "All errors must be wrapped as *GitError with captured stderr and command args".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@service.go` around lines 240 - 247, The validatePath() WorkDir validation
currently returns coreerr.E(...) which breaks the package contract that all
service errors must be *GitError; change both failure branches (non-absolute
WorkDir and path outside WorkDir) to return a *GitError instead by wrapping the
core error using the package's GitError constructor/helper used elsewhere
(search for NewGitError/MakeGitError or a wrapGitError helper) so the returned
error preserves Args and Stderr fields consistent with other git errors from
this package (keep references to validatePath, WorkDir, and the coreerr.E
message text when wrapping).
…andling - go.mod: rename module to dappco.re/go/git per RFC canonical path - docs + CLAUDE.md + llms.txt + README.md: match new module path - service.go: unsupported broadcast/query messages ignored silently instead of logging spurious errors - service_extra_test.go: updated to reflect new no-op behavior Verified: GOWORK=off go test ./... passes No current consumers of old path detected in /Users/snider/Code/core/. Co-Authored-By: Virgil <virgil@lethean.io>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@service.go`:
- Around line 105-119: The type assertions for opts.Get("paths").Value and
opts.Get("names").Value in the actionGitPushMultiple and actionGitPullMultiple
handlers are ignoring the boolean ok and can silently yield nil slices/maps;
change both handlers to check the assertion result (e.g., paths, ok :=
r.Value.([]string) and names, ok := r.Value.(map[string]string)) and if the
assertion fails return an error core.Result (or a descriptive failure) instead
of calling runPushMultiple/runPullMultiple; ensure any error path uses the same
logging/return style your service expects and update callers runPushMultiple and
runPullMultiple only if you need different parameter types or clearer error
propagation.
🪄 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: 267e9f8a-e910-4976-8d28-a8dc68b3c885
📒 Files selected for processing (9)
CLAUDE.mdREADME.mddocs/architecture.mddocs/development.mddocs/index.mdgo.modllms.txtservice.goservice_extra_test.go
✅ Files skipped from review due to trivial changes (1)
- CLAUDE.md
🚧 Files skipped from review as they are similar to previous changes (2)
- go.mod
- docs/index.md
Comment on lines
+105
to
+119
| s.Core().Action(actionGitPushMultiple, func(ctx context.Context, opts core.Options) core.Result { | ||
| r := opts.Get("paths") | ||
| paths, _ := r.Value.([]string) | ||
| r = opts.Get("names") | ||
| names, _ := r.Value.(map[string]string) | ||
| return s.runPushMultiple(ctx, paths, names) | ||
| }) | ||
|
|
||
| s.Core().Action(actionGitPullMultiple, func(ctx context.Context, opts core.Options) core.Result { | ||
| r := opts.Get("paths") | ||
| paths, _ := r.Value.([]string) | ||
| r = opts.Get("names") | ||
| names, _ := r.Value.(map[string]string) | ||
| return s.runPullMultiple(ctx, paths, names) | ||
| }) |
There was a problem hiding this comment.
Silent type assertion failures could cause nil paths/names.
The type assertions use the blank identifier to discard the success boolean:
paths, _ := r.Value.([]string)
names, _ := r.Value.(map[string]string)If the options contain incorrect types (e.g., []interface{} instead of []string), the assertion silently fails and paths becomes nil. The downstream validatePaths(paths) would then succeed (empty slice), and PushMultiple/PullMultiple would return empty results without indicating the input error.
Consider validating the type assertion or returning an error when the expected types are not found.
🛡️ Proposed fix to handle type assertion failures
s.Core().Action(actionGitPushMultiple, func(ctx context.Context, opts core.Options) core.Result {
r := opts.Get("paths")
- paths, _ := r.Value.([]string)
+ paths, ok := r.Value.([]string)
+ if !ok {
+ return s.Core().LogError(coreerr.E("git.push-multiple", "paths must be []string", nil), "git.push-multiple", "invalid paths type")
+ }
r = opts.Get("names")
- names, _ := r.Value.(map[string]string)
+ names, _ := r.Value.(map[string]string) // names is optional, nil is acceptable
return s.runPushMultiple(ctx, paths, names)
})Apply similar changes to actionGitPullMultiple.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@service.go` around lines 105 - 119, The type assertions for
opts.Get("paths").Value and opts.Get("names").Value in the actionGitPushMultiple
and actionGitPullMultiple handlers are ignoring the boolean ok and can silently
yield nil slices/maps; change both handlers to check the assertion result (e.g.,
paths, ok := r.Value.([]string) and names, ok := r.Value.(map[string]string))
and if the assertion fails return an error core.Result (or a descriptive
failure) instead of calling runPushMultiple/runPullMultiple; ensure any error
path uses the same logging/return style your service expects and update callers
runPushMultiple and runPullMultiple only if you need different parameter types
or clearer error propagation.
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.
Module path migration from forge.lthn.ai to dappco.re
Summary by CodeRabbit
New Features
Bug Fixes
Chores