feat: add type checking infrastructure for viruses - #34
Conversation
📝 WalkthroughSummary by CodeRabbitRelease Notes
WalkthroughSwitches source discovery to golang.org/x/tools/go/packages with type-checked ASTs cached in GoSourceFile; adds Parse and equality helpers; updates repositories, tests, and fake repos to use parsed GoSourceFile objects; and enables the cancelnil virus in default release options. Changes
Sequence Diagram(s)sequenceDiagram
participant Test as Test/Runner
participant FSRepo as FSRepository
participant GoPkgs as "go/packages"
participant GoFile as GoSourceFile
participant Virus as VirusIncubator
Test->>FSRepo: ListGoSourceFiles(root)
FSRepo->>GoPkgs: packages.Load(cfg, root) (Fset, Syntax, TypesInfo)
GoPkgs-->>FSRepo: packages
FSRepo->>GoFile: Parse(relativePath, rawContent) / New(...cached AST & types)
FSRepo-->>Test: []*GoSourceFile
Test->>GoFile: Incubate(virus)
GoFile->>Virus: virus.Incubate(ast, typeInfo)
Virus-->>GoFile: mutated AST/files
Test->>Release: run with defaultOptions (includes cancelnil)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 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. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
✨ Simplify code
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
viruses/cancelnil/cancelnil.go (1)
39-43:⚠️ Potential issue | 🟡 MinorAdd bounds check before accessing
call.Args[0].If the source code contains a malformed call like
cancel()(missing argument), accessingcall.Args[0]will panic. While the compiler would normally catch this, mutation testing may process code with syntax errors.🛡️ Proposed defensive fix
+ if len(call.Args) == 0 { + return nil + } + origArg := call.Args[0]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@viruses/cancelnil/cancelnil.go` around lines 39 - 43, The code assumes call.Args[0] exists which will panic for zero-argument calls; add a bounds check before accessing it (e.g. check len(call.Args) == 0 or len(call.Args) < 1) and return nil early if there are no arguments so the subsequent logic that inspects origArg / the *ast.Ident named "nil" is safe; update the block around origArg := call.Args[0] to first guard on call.Args length and then proceed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/fsrepository/fsrepository.go`:
- Around line 106-109: The code silently skips files when os.ReadFile(absPath)
returns an error, which can hide permission or transient deletion issues; update
the error handling in the file-reading loop (the block using os.ReadFile,
rawContent and absPath) to log the error at a verbose/debug level (including
absPath and err) instead of just continuing—use the repository's existing logger
if available, or add a conditional verbose log path so normal runs stay quiet
but developers can see which files were skipped.
- Around line 61-64: packages.Load can return packages with per-package errors
in pkg.Errors even when err == nil; update the code after pkgs, err :=
packages.Load(cfg, "./...") to iterate over pkgs and check each pkg.Errors (or
pkg.CompiledGoFiles/Types) and handle/skip packages with errors: for each pkg in
pkgs inspect pkg.Errors and either log warnings via the existing logger and
continue (to support graceful degradation) or return the error immediately if
full correctness is required; reference the variables/packages.Load call and the
pkg.Errors field to locate where to add the per-package validation and
logging/early-return logic.
In `@oozetesting/test.go`:
- Around line 117-118: The call to conf.Check currently discards its error;
update the call to capture the error returned by conf.Check (e.g., err :=
conf.Check(...)) and if err != nil log the error with context (using the
available logger/test helper such as t.Logf or log.Printf/ fmt.Printf) while
preserving existing graceful degradation (still proceed with fset/file/info).
Locate the conf variable (types.Config{Importer: importer.Default()}) and the
conf.Check invocation to implement this change.
In `@release.go`:
- Line 62: The cancelnil virus (cancelnil) currently does nothing when type
information is nil but this silent skip is undocumented; update the Incubate
method on the cancelnil type to either add a clear comment stating that typeInfo
is required for generating mutations and why, or add a debug/process logger line
inside Incubate that emits a message when typeInfo == nil (e.g., "cancelnil:
skipping mutation generation due to missing typeInfo") so callers can see when
the virus is ineffective; ensure the log/comment references the typeInfo check
and keep the behavior unchanged otherwise.
---
Outside diff comments:
In `@viruses/cancelnil/cancelnil.go`:
- Around line 39-43: The code assumes call.Args[0] exists which will panic for
zero-argument calls; add a bounds check before accessing it (e.g. check
len(call.Args) == 0 or len(call.Args) < 1) and return nil early if there are no
arguments so the subsequent logic that inspects origArg / the *ast.Ident named
"nil" is safe; update the block around origArg := call.Args[0] to first guard on
call.Args length and then proceed.
🪄 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: ASSERTIVE
Plan: Pro Plus
Run ID: 1aa99bb4-1d0c-437b-800d-161a58bb8e7e
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (7)
go.modinternal/fsrepository/fsrepository.gointernal/gosourcefile/gosourcefile.gooozetesting/test.gorelease.goviruses/cancelnil/cancelnil.goviruses/cancelnil/cancelnil_test.go
| arithmeticassignment.New(), | ||
| arithmeticassignmentinvert.New(), | ||
| bitwise.New(), | ||
| cancelnil.New(), |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check for documentation or logging about type-info requirements
rg -n -C2 "typeInfo.*nil|type.*check|typed.*load" --type=goRepository: gtramontina/ooze
Length of output: 633
🏁 Script executed:
#!/bin/bash
# Check for package-level documentation or comments about type requirements
echo "=== Checking cancelnil package file structure ==="
ls -la viruses/cancelnil/
echo ""
echo "=== Checking for README or doc files mentioning type-info ==="
find . -maxdepth 3 -type f \( -name "README*" -o -name "*.md" \) -exec grep -l "type.*info\|cancelnil" {} \;
echo ""
echo "=== Checking for comments in cancelnil.go ==="
head -30 viruses/cancelnil/cancelnil.goRepository: gtramontina/ooze
Length of output: 1070
🏁 Script executed:
#!/bin/bash
# Search for documentation about type-info requirements in readme
rg -n -C3 "type" readme.md | head -50Repository: gtramontina/ooze
Length of output: 1453
Ensure cancelnil behavior is documented when type information is unavailable.
The cancelnil virus silently produces no mutations when typeInfo is nil (lines 21-23), but this behavior is undocumented. Since type information may be unavailable in certain scenarios, users won't know when this virus is ineffective.
Add a comment to the Incubate method explaining why typeInfo is required, or emit a debug log when the virus is skipped due to missing type information.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@release.go` at line 62, The cancelnil virus (cancelnil) currently does
nothing when type information is nil but this silent skip is undocumented;
update the Incubate method on the cancelnil type to either add a clear comment
stating that typeInfo is required for generating mutations and why, or add a
debug/process logger line inside Incubate that emits a message when typeInfo ==
nil (e.g., "cancelnil: skipping mutation generation due to missing typeInfo") so
callers can see when the virus is ineffective; ensure the log/comment references
the typeInfo check and keep the behavior unchanged otherwise.
6418aeb to
46e8722
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
viruses/cancelnil/cancelnil_test.go (1)
11-31:⚠️ Potential issue | 🟠 MajorUse keyed struct literals for
Mutationsvalues to satisfy typecheck.The struct definition requires explicit field names. Replace the positional struct literals with keyed literals using
SourceFileNameandMutantFileNames.Suggested fix
scenarios := oozetesting.NewScenarios( "Call cancel(nil)", cancelnil.New(), oozetesting.Mutations{ - "no mutations": {"source.0.go", []string{}}, - "one mutation": {"source.1.go", []string{ - "source.1.mut.1.go", - }}, - "two mutations": {"source.2.go", []string{ - "source.2.mut.1.go", - "source.2.mut.2.go", - }}, - "many mutations": {"source.3.go", []string{ - "source.3.mut.1.go", - "source.3.mut.2.go", - "source.3.mut.3.go", - }}, + "no mutations": { + SourceFileName: "source.0.go", + MutantFileNames: []string{}, + }, + "one mutation": { + SourceFileName: "source.1.go", + MutantFileNames: []string{ + "source.1.mut.1.go", + }, + }, + "two mutations": { + SourceFileName: "source.2.go", + MutantFileNames: []string{ + "source.2.mut.1.go", + "source.2.mut.2.go", + }, + }, + "many mutations": { + SourceFileName: "source.3.go", + MutantFileNames: []string{ + "source.3.mut.1.go", + "source.3.mut.2.go", + "source.3.mut.3.go", + }, + }, }, )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@viruses/cancelnil/cancelnil_test.go` around lines 11 - 31, The Mutations map values use positional struct literals that no longer typecheck; update the entries in the oozetesting.NewScenarios call so each value is a keyed struct literal with the fields SourceFileName and MutantFileNames (e.g. replace {"source.0.go", []string{}} with {SourceFileName: "source.0.go", MutantFileNames: []string{}}) for all keys ("no mutations", "one mutation", "two mutations", "many mutations") so the Mutations type used by cancelnil.New and oozetesting.NewScenarios compiles; leave scenarios.TypeCheck = true and oozetesting.Run(t, scenarios) as-is.
♻️ Duplicate comments (2)
internal/fsrepository/fsrepository.go (1)
61-71:⚠️ Potential issue | 🟠 MajorFall back when
packages.Loadreports package errors.
err == nilis not enough here.packages.Loadcan still populatepkg.Errors, and thenpkg.Syntax/pkg.TypesInfomay be incomplete. In that case this method returns a partial typed file list and never reaches the filesystem fallback, so some source files can disappear from mutation runs.Suggested fix
pkgs, err := packages.Load(cfg, "./...") if err != nil || len(pkgs) == 0 { return nil } + for _, pkg := range pkgs { + if len(pkg.Errors) > 0 { + return nil + } + } + var sourceFiles []*gosourcefile.GoSourceFile seen := make(map[string]bool)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/fsrepository/fsrepository.go` around lines 61 - 71, packages.Load may return pkgs with non-empty pkg.Errors even when err == nil, causing collectPackageFiles to process incomplete Syntax/TypesInfo and preventing the filesystem fallback; update the load-check in the function that calls packages.Load so that if err != nil or len(pkgs) == 0 or any pkg in pkgs has len(pkg.Errors) > 0 you treat it as a load failure (return nil to trigger the filesystem fallback). Specifically, after calling packages.Load check each pkg.Errors and bail out before calling r.collectPackageFiles (the collectPackageFiles call should only run when packages.Load produced no package errors).oozetesting/test.go (1)
117-120:⚠️ Potential issue | 🟠 MajorOnly keep the typed path when module-aware checking succeeds.
This helper still uses
importer.Default()—the resolver this PR is moving away from for internal/module-local imports—and it ignoresconf.Checkerrors before returningNewTyped(...). That means typed virus tests can run with partialtypes.Infoinstead of cleanly falling back to the untyped path.Use the same
packages.Load-based strategy here, or at minimum returngosourcefile.New(...)whenCheckfails.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@oozetesting/test.go` around lines 117 - 120, The current helper uses types.Config{Importer: importer.Default()} and ignores errors from conf.Check before returning gosourcefile.NewTyped, which can produce partial types.Info; change this so module-aware typed checking is only used when checking succeeds: either adopt the packages.Load-based strategy used elsewhere to obtain a proper module-aware type-check (preferred) or, at minimum, detect conf.Check's returned error and if it fails return gosourcefile.New(...) instead of gosourcefile.NewTyped(...). Replace importer.Default usage with the packages.Load approach or a module-aware importer when using types.Config, and ensure you only call gosourcefile.NewTyped(fileName, source, fset, file, info) when conf.Check succeeded.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@viruses/cancelnil/cancelnil_test.go`:
- Around line 11-31: The Mutations map values use positional struct literals
that no longer typecheck; update the entries in the oozetesting.NewScenarios
call so each value is a keyed struct literal with the fields SourceFileName and
MutantFileNames (e.g. replace {"source.0.go", []string{}} with {SourceFileName:
"source.0.go", MutantFileNames: []string{}}) for all keys ("no mutations", "one
mutation", "two mutations", "many mutations") so the Mutations type used by
cancelnil.New and oozetesting.NewScenarios compiles; leave scenarios.TypeCheck =
true and oozetesting.Run(t, scenarios) as-is.
---
Duplicate comments:
In `@internal/fsrepository/fsrepository.go`:
- Around line 61-71: packages.Load may return pkgs with non-empty pkg.Errors
even when err == nil, causing collectPackageFiles to process incomplete
Syntax/TypesInfo and preventing the filesystem fallback; update the load-check
in the function that calls packages.Load so that if err != nil or len(pkgs) == 0
or any pkg in pkgs has len(pkg.Errors) > 0 you treat it as a load failure
(return nil to trigger the filesystem fallback). Specifically, after calling
packages.Load check each pkg.Errors and bail out before calling
r.collectPackageFiles (the collectPackageFiles call should only run when
packages.Load produced no package errors).
In `@oozetesting/test.go`:
- Around line 117-120: The current helper uses types.Config{Importer:
importer.Default()} and ignores errors from conf.Check before returning
gosourcefile.NewTyped, which can produce partial types.Info; change this so
module-aware typed checking is only used when checking succeeds: either adopt
the packages.Load-based strategy used elsewhere to obtain a proper module-aware
type-check (preferred) or, at minimum, detect conf.Check's returned error and if
it fails return gosourcefile.New(...) instead of gosourcefile.NewTyped(...).
Replace importer.Default usage with the packages.Load approach or a module-aware
importer when using types.Config, and ensure you only call
gosourcefile.NewTyped(fileName, source, fset, file, info) when conf.Check
succeeded.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 9372dd9d-ea72-4c7f-b2ad-79d2fd1c3557
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (7)
go.modinternal/fsrepository/fsrepository.gointernal/gosourcefile/gosourcefile.gooozetesting/test.gorelease.goviruses/cancelnil/cancelnil.goviruses/cancelnil/cancelnil_test.go
46e8722 to
f4448aa
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/fsrepository/fsrepository.go (1)
149-152:⚠️ Potential issue | 🟡 MinorPre-existing issue: unhandled
ReadFileerror could cause downstream panic.In the fallback path, if
os.ReadFile(path)fails,datawill benilbut the file is still added tosourceFiles. WhenIncubatelater attempts to parse this empty content, it will panic atgosourcefile.go:58.🛡️ Proposed fix to handle ReadFile errors
for i, path := range paths { - data, _ := os.ReadFile(path) + data, err := os.ReadFile(path) + if err != nil { + continue + } relativePath, _ := filepath.Rel(r.root, path) sourceFiles[i] = gosourcefile.New(relativePath, data) }Note: This fix introduces a potential issue with sparse
sourceFilesslice. A cleaner approach:- sourceFiles := make([]*gosourcefile.GoSourceFile, len(paths)) - - for i, path := range paths { - data, _ := os.ReadFile(path) + var sourceFiles []*gosourcefile.GoSourceFile + + for _, path := range paths { + data, err := os.ReadFile(path) + if err != nil { + continue + } relativePath, _ := filepath.Rel(r.root, path) - sourceFiles[i] = gosourcefile.New(relativePath, data) + sourceFiles = append(sourceFiles, gosourcefile.New(relativePath, data)) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/fsrepository/fsrepository.go` around lines 149 - 152, The loop currently ignores errors from os.ReadFile and filepath.Rel, which can leave nil/empty entries in sourceFiles and cause a panic later in Incubate/gosourcefile.New; modify the loop to check the errors from os.ReadFile(path) and filepath.Rel(r.root, path) and fail fast by returning an error (wrap with context including the path) instead of inserting a nil/empty entry into sourceFiles, ensuring sourceFiles is only populated with successfully read files.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@internal/fsrepository/fsrepository.go`:
- Around line 149-152: The loop currently ignores errors from os.ReadFile and
filepath.Rel, which can leave nil/empty entries in sourceFiles and cause a panic
later in Incubate/gosourcefile.New; modify the loop to check the errors from
os.ReadFile(path) and filepath.Rel(r.root, path) and fail fast by returning an
error (wrap with context including the path) instead of inserting a nil/empty
entry into sourceFiles, ensuring sourceFiles is only populated with successfully
read files.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: a2b515c0-ca3e-41ff-b5ec-f69a22685de7
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (7)
go.modinternal/fsrepository/fsrepository.gointernal/gosourcefile/gosourcefile.gooozetesting/test.gorelease.goviruses/cancelnil/cancelnil.goviruses/cancelnil/cancelnil_test.go
f4448aa to
8c027f3
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
oozetesting/test.go (1)
103-120:⚠️ Potential issue | 🟠 MajorDon't silently downgrade typed scenarios to partial type info.
TypeCheckis opt-in, but this helper still usesimporter.Default()and ignoresconf.Checkerrors before returninggosourcefile.NewTyped(...). That reintroduces the same unresolved-import weakness this PR removes in production, and tests can keep running against incompletetypes.Infoinstead of failing fast. Please either switch this helper to the samego/packages-based loading path asinternal/fsrepository/fsrepository.go, or make type-check failure fail the test immediately.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@oozetesting/test.go` around lines 103 - 120, The helper typeCheckSourceFile currently calls conf.Check(... ) with importer.Default() and ignores its returned error, which allows tests to continue with incomplete types.Info; update typeCheckSourceFile to check the error returned by conf.Check and fail fast instead of returning a typed GoSourceFile on error (e.g., change the helper to accept a *testing.T and call t.Fatalf or propagate the error to the caller), ensuring you do not silently fall back to gosourcefile.NewTyped when conf.Check fails; keep references to the same identifiers (typeCheckSourceFile, conf.Check, importer.Default, gosourcefile.NewTyped) so the change is easy to locate.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@oozetesting/test.go`:
- Around line 103-120: The helper typeCheckSourceFile currently calls
conf.Check(... ) with importer.Default() and ignores its returned error, which
allows tests to continue with incomplete types.Info; update typeCheckSourceFile
to check the error returned by conf.Check and fail fast instead of returning a
typed GoSourceFile on error (e.g., change the helper to accept a *testing.T and
call t.Fatalf or propagate the error to the caller), ensuring you do not
silently fall back to gosourcefile.NewTyped when conf.Check fails; keep
references to the same identifiers (typeCheckSourceFile, conf.Check,
importer.Default, gosourcefile.NewTyped) so the change is easy to locate.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 08997c1e-bd4d-495d-83d9-5301ae0e6435
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (7)
go.modinternal/fsrepository/fsrepository.gointernal/gosourcefile/gosourcefile.gooozetesting/test.gorelease.goviruses/cancelnil/cancelnil.goviruses/cancelnil/cancelnil_test.go
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/fsrepository/fsrepository_test.go`:
- Line 7: Remove the unused reflect import and the no-op sentinel declaration
(the import "reflect" and the line var _ =
reflect.TypeOf([]*gosourcefile.GoSourceFile{})); delete both from the test file
and run `go vet`/`go test` to confirm there are no other references to reflect
or gosourcefile.GoSourceFile so the build and linters remain clean.
- Around line 15-32: Extract the duplicated test helpers equalGoSourceFiles and
assertGoSourceFilesEqual into a shared test utility package (e.g.,
internal/oozetesting/gosourcefiletest), move their implementations there and
export a single entrypoint function (e.g., AssertGoSourceFilesEqual) or keep
both helpers exported as needed, update imports in
internal/fsrepository/fsrepository_test.go and
internal/ignoredrepository/ignoredrepository_test.go to import the new package,
and replace the local helper calls with calls to the shared helpers (referencing
equalGoSourceFiles / assertGoSourceFilesEqual or the new exported
AssertGoSourceFilesEqual) so both tests use the centralized implementation.
In `@internal/fsrepository/fsrepository.go`:
- Around line 86-89: The defensive bounds check in the loop iterating pkg.Syntax
vs pkg.CompiledGoFiles should remain, but add a debug/error log when the
mismatch happens to surface unexpected package-loading state; inside the loop
where it currently does "if i >= len(pkg.CompiledGoFiles) { continue }", log
(using the repository's logger or existing logging facility) the index i,
len(pkg.CompiledGoFiles), the current file name (file) and package identifier
(pkg.PkgPath or pkg.ID) so maintainers can trace why pkg.Syntax and
pkg.CompiledGoFiles are out of sync while still continuing past the entry.
In `@internal/ignoredrepository/ignoredrepository_test.go`:
- Line 4: Remove the unused "reflect" import from ignoredrepository_test.go and
delete the no-op/unused statement at the test file's line 33 (the dead code that
mirrors the same issue in fsrepository_test.go); ensure imports are cleaned up
and tests compile without the unused reflect reference.
🪄 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: ASSERTIVE
Plan: Pro Plus
Run ID: 77e31219-b9b4-45c2-b883-1457e18e54b4
📒 Files selected for processing (8)
internal/fsrepository/fsrepository.gointernal/fsrepository/fsrepository_test.gointernal/gosourcefile/gosourcefile.gointernal/ignoredrepository/ignoredrepository_test.gointernal/oozetesting/fakerepository/fakerepository.gointernal/verboserepository/verboserepository_test.gooozetesting/test.goviruses/cancelnil/cancelnil_test.go
💤 Files with no reviewable changes (1)
- viruses/cancelnil/cancelnil_test.go
| "io/fs" | ||
| "os" | ||
| "path/filepath" | ||
| "reflect" |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Unnecessary reflect import and dead code.
Line 34 (var _ = reflect.TypeOf([]*gosourcefile.GoSourceFile{})) appears to be a no-op that doesn't serve any functional purpose. If this was added to suppress an unused import warning or satisfy a linter, consider removing both the import and this line.
♻️ Proposed removal
import (
"io/fs"
"os"
"path/filepath"
- "reflect"
"testing"
"github.com/gtramontina/ooze/internal/fsrepository"
"github.com/gtramontina/ooze/internal/gosourcefile"
"github.com/stretchr/testify/assert"
)-var _ = reflect.TypeOf([]*gosourcefile.GoSourceFile{})
-
func TestFSRepository(t *testing.T) {Also applies to: 34-34
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/fsrepository/fsrepository_test.go` at line 7, Remove the unused
reflect import and the no-op sentinel declaration (the import "reflect" and the
line var _ = reflect.TypeOf([]*gosourcefile.GoSourceFile{})); delete both from
the test file and run `go vet`/`go test` to confirm there are no other
references to reflect or gosourcefile.GoSourceFile so the build and linters
remain clean.
| func equalGoSourceFiles(first, second []*gosourcefile.GoSourceFile) bool { | ||
| if len(first) != len(second) { | ||
| return false | ||
| } | ||
|
|
||
| for i := range first { | ||
| if !first[i].Equal(second[i]) { | ||
| return false | ||
| } | ||
| } | ||
|
|
||
| return true | ||
| } | ||
|
|
||
| func assertGoSourceFilesEqual(t *testing.T, expected, actual []*gosourcefile.GoSourceFile) { | ||
| t.Helper() | ||
| assert.True(t, equalGoSourceFiles(expected, actual), "expected: %v, actual: %v", expected, actual) | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider extracting shared test helpers to reduce duplication.
The equalGoSourceFiles and assertGoSourceFilesEqual functions are duplicated in internal/ignoredrepository/ignoredrepository_test.go. Consider extracting them to a shared test utility package (e.g., internal/oozetesting/gosourcefiletest/).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/fsrepository/fsrepository_test.go` around lines 15 - 32, Extract the
duplicated test helpers equalGoSourceFiles and assertGoSourceFilesEqual into a
shared test utility package (e.g., internal/oozetesting/gosourcefiletest), move
their implementations there and export a single entrypoint function (e.g.,
AssertGoSourceFilesEqual) or keep both helpers exported as needed, update
imports in internal/fsrepository/fsrepository_test.go and
internal/ignoredrepository/ignoredrepository_test.go to import the new package,
and replace the local helper calls with calls to the shared helpers (referencing
equalGoSourceFiles / assertGoSourceFilesEqual or the new exported
AssertGoSourceFilesEqual) so both tests use the centralized implementation.
| for i, file := range pkg.Syntax { | ||
| if i >= len(pkg.CompiledGoFiles) { | ||
| continue | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Defensive bounds check is reasonable but may indicate an unexpected state.
According to go/packages documentation, pkg.Syntax[i] should correspond to pkg.CompiledGoFiles[i]. A mismatch would indicate package loading issues. The defensive check prevents panics, but you might consider logging when this condition occurs to aid debugging.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/fsrepository/fsrepository.go` around lines 86 - 89, The defensive
bounds check in the loop iterating pkg.Syntax vs pkg.CompiledGoFiles should
remain, but add a debug/error log when the mismatch happens to surface
unexpected package-loading state; inside the loop where it currently does "if i
>= len(pkg.CompiledGoFiles) { continue }", log (using the repository's logger or
existing logging facility) the index i, len(pkg.CompiledGoFiles), the current
file name (file) and package identifier (pkg.PkgPath or pkg.ID) so maintainers
can trace why pkg.Syntax and pkg.CompiledGoFiles are out of sync while still
continuing past the entry.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
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 `@internal/fsrepository/fsrepository_test.go`:
- Around line 55-57: Add a regression test case in fsrepository_test.go that
verifies fallback discovery when no go.mod is present: create a temp dir (using
t.TempDir()), do NOT write a go.mod file, create one or more .go files under
that dir, instantiate the repository with fsrepository.New(dir), call
repository.ListGoSourceFiles (or the test helper that exercises it), and assert
that the returned list includes the created .go files; place this alongside the
existing module-mode test so the fallback behavior is covered.
In `@internal/fsrepository/fsrepository.go`:
- Around line 52-60: The current code returns nil when packages.Load fails or
any loaded package has errors, which prevents the non-module filesystem
fallback; change the logic around the packages.Load result (the pkgs variable
and pkg.Errors checks) to avoid returning nil on error — instead log the
load/error details and continue to the existing filesystem walk fallback path so
discovery still finds files for non-module repos or when some packages are
unloadable. Ensure you keep the packages.Load attempt (packages.Load) but treat
its failure or pkg.Errors > 0 as a signal to use the fallback discovery rather
than aborting.
In `@internal/gosourcefile/gosourcefile.go`:
- Around line 28-29: The current Parse uses types.Config with importer.Default()
and calls conf.Check on a single file (symbols: Parse, types.Config,
importer.Default(), conf.Check), which only yields GOPATH-mode, single-file type
info; change Parse to load package-level, module-aware type information via
golang.org/x/tools/go/packages (use packages.Load with Mode including NeedTypes
and NeedTypesInfo, set Fset/Dir appropriately) and then use the returned
package's TypesInfo instead of conf.Check so cross-file and module-local symbols
are resolved (follow the same package-loading approach used by FSRepository).
🪄 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: ASSERTIVE
Plan: Pro Plus
Run ID: e692f579-02d8-4949-b9e7-c0cdf7b16b4c
📒 Files selected for processing (8)
internal/fsrepository/fsrepository.gointernal/fsrepository/fsrepository_test.gointernal/gosourcefile/gosourcefile.gointernal/ignoredrepository/ignoredrepository_test.gointernal/oozetesting/fakerepository/fakerepository.gointernal/verboserepository/verboserepository_test.gooozetesting/test.goviruses/cancelnil/cancelnil_test.go
💤 Files with no reviewable changes (1)
- viruses/cancelnil/cancelnil_test.go
| dir := t.TempDir() | ||
| assert.NoError(t, os.WriteFile(dir+"/go.mod", []byte("module m\n"), 0o600)) | ||
| repository := fsrepository.New(dir) |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add a no-go.mod regression case.
This file now forces module mode in every ListGoSourceFiles scenario, so the PR's fallback behavior is untested. Please add one case that omits go.mod and still expects discovered .go files once the fallback path is fixed.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/fsrepository/fsrepository_test.go` around lines 55 - 57, Add a
regression test case in fsrepository_test.go that verifies fallback discovery
when no go.mod is present: create a temp dir (using t.TempDir()), do NOT write a
go.mod file, create one or more .go files under that dir, instantiate the
repository with fsrepository.New(dir), call repository.ListGoSourceFiles (or the
test helper that exercises it), and assert that the returned list includes the
created .go files; place this alongside the existing module-mode test so the
fallback behavior is covered.
| conf := types.Config{Importer: importer.Default()} //nolint:exhaustruct | ||
| _, _ = conf.Check("source", fset, []*ast.File{file}, info) |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
How does go/types importer.Default resolve module-local or internal package imports, and what type information is available when types.Config.Check is called with only one *ast.File from a multi-file package?
💡 Result:
importer.Default returns an Importer for the compiler that built the running binary (typically "gc"), implementing types.ImporterFrom. For module-local or internal package imports (e.g., "internal/foo" or same-module paths like "example.com/my/module/pkg"), it calls gcimporter.Import(path, srcDir, ...), where srcDir is the directory of the file being checked (from Config.Check's fset). gcimporter uses go/build.Context (GOPATH-mode) to resolve paths via build.Import, looking in GOPATH/src, module cache, or srcDir for vendoring. It does not use full module resolution; for proper module-aware import, provide a custom Lookup func or use golang.org/x/tools/go/packages.Load. When types.Config.Check is called with only one *ast.File from a multi-file package, it type-checks solely that file's contents. Type information available includes: resolved types for expressions in that file (via Info.Types), defined objects (Info.Defs), used objects (Info.Uses), scopes (Info.Scopes), etc., for the provided AST. Definitions/uses in other unprovided files are unavailable (nil objects), and references to them may cause errors or incomplete info. The resulting *types.Package has Scope containing only objects declared in the single file. Imported packages are resolved normally. This works for single-file packages or partial checking, as shown in official gotypes tutorial example with one file. Note: go/importer is deprecated for production module-aware use post-Go 1.11; prefer x/tools/go/packages for complete packages.
Citations:
- 1: https://pkg.go.dev/go/importer
- 2: https://godoc.org/go/types?tab=imports
- 3: https://go.dev/src/go/importer/importer.go
- 4: https://github.com/golang/example/blob/master/gotypes/README.md
- 5: https://pkg.go.dev/go/types@go1.26.0
- 6: https://pkg.go.dev/go/types
- 7: https://pkg.go.dev/go/internal/gcimporter@go1.18.3
- 8: https://github.com/golang/go/blob/master/src/go/importer/importer.go
🏁 Script executed:
# Check the full gosourcefile.go file to understand context
wc -l internal/gosourcefile/gosourcefile.goRepository: gtramontina/ooze
Length of output: 101
🏁 Script executed:
# Search for references to types.Config or importer in the codebase
rg "types\.Config|importer\." --type goRepository: gtramontina/ooze
Length of output: 171
🏁 Script executed:
# Search for golang.org/x/tools/go/packages references (module-aware alternative)
rg "packages\." --type go -A 2 -B 2Repository: gtramontina/ooze
Length of output: 1210
🏁 Script executed:
# Check if there's an alternative implementation or discussion about module-aware loading
rg "package.*aware|module.*aware|gcimporter|Lookup" --type goRepository: gtramontina/ooze
Length of output: 42
🏁 Script executed:
# Read gosourcefile.go to understand Parse's purpose and context
cat -n internal/gosourcefile/gosourcefile.goRepository: gtramontina/ooze
Length of output: 2687
🏁 Script executed:
# Check oozetesting/test.go around line 53
sed -n '45,60p' oozetesting/test.go | cat -nRepository: gtramontina/ooze
Length of output: 634
🏁 Script executed:
# Check fakerepository.go around line 49
sed -n '40,60p' internal/oozetesting/fakerepository/fakerepository.go | cat -nRepository: gtramontina/ooze
Length of output: 812
Parse does not provide package-aware type information, limiting completeness for type-aware viruses.
Parse at lines 28–29 uses importer.Default() (which provides GOPATH-mode import resolution only) and type-checks only the single file passed to conf.Check(). This means cross-file and module-local symbol references are unavailable in the resulting type info. Tests that call Parse (e.g., oozetesting/test.go line 9, internal/oozetesting/fakerepository/fakerepository.go line 10) therefore exercise a limited type-checking path—not the module-aware one that FSRepository uses via packages.Load(). Type-aware viruses like cancelnil, which depend on complete type information, may miss mutations in code patterns where cross-file type resolution matters.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/gosourcefile/gosourcefile.go` around lines 28 - 29, The current
Parse uses types.Config with importer.Default() and calls conf.Check on a single
file (symbols: Parse, types.Config, importer.Default(), conf.Check), which only
yields GOPATH-mode, single-file type info; change Parse to load package-level,
module-aware type information via golang.org/x/tools/go/packages (use
packages.Load with Mode including NeedTypes and NeedTypesInfo, set Fset/Dir
appropriately) and then use the returned package's TypesInfo instead of
conf.Check so cross-file and module-local symbols are resolved (follow the same
package-loading approach used by FSRepository).
86a14c8 to
430ea7a
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (5)
internal/gosourcefile/gosourcefile.go (1)
28-29:⚠️ Potential issue | 🟠 Major
Parsestill builds single-file GOPATH-style type info.
importer.Default()plusconf.Check(..., []*ast.File{file}, ...)does not produce the same module-aware, package-levelTypesInfothatinternal/fsrepository/fsrepository.gonow gets frompackages.Load. Anything that goes throughgosourcefile.Parse—notablyoozetesting/test.goandinternal/oozetesting/fakerepository/fakerepository.go—still exercises a partial typing path, socancelnilcan miss matches that depend on other files or module-local imports.#!/bin/bash set -euo pipefail echo "=== gosourcefile.Parse ===" sed -n '1,80p' internal/gosourcefile/gosourcefile.go echo echo "=== fsrepository.ListGoSourceFiles ===" sed -n '1,140p' internal/fsrepository/fsrepository.go echo echo "=== loader usage across the repo ===" rg -n "importer.Default|packages.Load|NeedTypesInfo|NeedSyntax" --type goExpect to see
gosourcefile.Parseusingimporter.Default()whileinternal/fsrepository/fsrepository.gousespackages.Load(...).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/gosourcefile/gosourcefile.go` around lines 28 - 29, The Parse function currently uses types.Config with importer.Default() and conf.Check (the conf.Check call in gosourcefile.Parse) which only builds single-file GOPATH-style type info; change Parse to use the packages loader (like internal/fsrepository/fsrepository.go) by creating a packages.Config with Mode including packages.NeedTypesInfo and packages.NeedSyntax (and any other modes needed), call packages.Load for the file's package/module context, locate the loaded package that contains the target AST file, and then extract the package-level TypesInfo and AST from that result instead of using conf.Check so module-aware type resolution and package-level info are produced.internal/fsrepository/fsrepository_test.go (1)
38-40:⚠️ Potential issue | 🟠 MajorAdd the no-
go.modregression here.Every
ListGoSourceFilesscenario in this file now opts into module mode by creatinggo.mod, so the fallback filesystem walk from the PR objective is never exercised. A broken non-module path would still pass this suite.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/fsrepository/fsrepository_test.go` around lines 38 - 40, The tests always create a go.mod which prevents exercising the non-module fallback; add a test case in fsrepository_test.go that does not write go.mod (i.e. use t.TempDir() and do not create go.mod) then instantiate repository with fsrepository.New(dir) and call ListGoSourceFiles (or the test helper that enumerates Go files) to assert it correctly falls back to a filesystem walk; ensure the new case references fsrepository.New and ListGoSourceFiles so the non-module code path is covered.internal/fsrepository/fsrepository.go (3)
86-89:⚠️ Potential issue | 🟡 MinorEmit a debug log when
pkg.Syntaxandpkg.CompiledGoFilesare out of sync.Line 87 silently continues on an unexpected loader state, which makes missing-file diagnosis hard.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/fsrepository/fsrepository.go` around lines 86 - 89, The loop iterating "for i, file := range pkg.Syntax" silently skips entries when "i >= len(pkg.CompiledGoFiles)"; add a debug log before the continue that records the package ID/name (or other package identifier available in this scope), the current index i, the lengths of pkg.Syntax and pkg.CompiledGoFiles, and optionally the first few filenames from pkg.CompiledGoFiles to help diagnosis; keep the existing continue behavior but ensure the log uses the existing logger instance used elsewhere in this file (or create one consistent with surrounding code) so out-of-sync conditions are visible during debugging.
52-60:⚠️ Potential issue | 🟠 MajorRestore filesystem fallback instead of returning
nilon package-load problems.Line 53 and Line 59 abort discovery entirely. That breaks the non-module/partial-load fallback path this PR is meant to preserve.
♻️ Proposed fix
pkgs, err := packages.Load(cfg, "./...") if err != nil || len(pkgs) == 0 { - return nil + return r.listGoSourceFilesFromFilesystem() } for _, pkg := range pkgs { if len(pkg.Errors) > 0 { - return nil + return r.listGoSourceFilesFromFilesystem() } }#!/bin/bash # Verify current control flow in ListGoSourceFiles still returns nil on load/errors rg -n -C2 'packages\.Load|len\(pkgs\) == 0|pkg\.Errors|return nil|listGoSourceFilesFromFilesystem' internal/fsrepository/fsrepository.go🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/fsrepository/fsrepository.go` around lines 52 - 60, The current early returns when packages.Load fails, when len(pkgs) == 0, or when any pkg.Errors exist remove the intended filesystem fallback; change these branches to invoke the filesystem fallback (call listGoSourceFilesFromFilesystem or the equivalent ListGoSourceFiles path) instead of returning nil so discovery continues when package loading is partial/non-module. Specifically, in the block around packages.Load and the loop over pkgs check for errors or empty pkg list and call listGoSourceFilesFromFilesystem(...) to collect files, and only return nil if both package-based discovery and the filesystem fallback fail.
103-105:⚠️ Potential issue | 🟡 MinorAvoid silently swallowing file read failures.
Line 105 drops files without any signal; this can hide permission/race issues and make mutation gaps opaque.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/fsrepository/fsrepository.go` around lines 103 - 105, The code silently ignores os.ReadFile errors (rawContent, err := os.ReadFile(absPath) followed by continue), which swallows permission/race failures; change this to propagate or log the error instead of continuing: replace the bare continue with explicit error handling that wraps and returns the error (e.g., return fmt.Errorf("read %s: %w", absPath, err)) or, if the function must continue, emit a structured log including absPath and err so failures are visible; update the function that contains this read (look for the reader that uses absPath/rawContent) to propagate the returned error up the call chain or record it via the repository logger.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@internal/fsrepository/fsrepository_test.go`:
- Around line 38-40: The tests always create a go.mod which prevents exercising
the non-module fallback; add a test case in fsrepository_test.go that does not
write go.mod (i.e. use t.TempDir() and do not create go.mod) then instantiate
repository with fsrepository.New(dir) and call ListGoSourceFiles (or the test
helper that enumerates Go files) to assert it correctly falls back to a
filesystem walk; ensure the new case references fsrepository.New and
ListGoSourceFiles so the non-module code path is covered.
In `@internal/fsrepository/fsrepository.go`:
- Around line 86-89: The loop iterating "for i, file := range pkg.Syntax"
silently skips entries when "i >= len(pkg.CompiledGoFiles)"; add a debug log
before the continue that records the package ID/name (or other package
identifier available in this scope), the current index i, the lengths of
pkg.Syntax and pkg.CompiledGoFiles, and optionally the first few filenames from
pkg.CompiledGoFiles to help diagnosis; keep the existing continue behavior but
ensure the log uses the existing logger instance used elsewhere in this file (or
create one consistent with surrounding code) so out-of-sync conditions are
visible during debugging.
- Around line 52-60: The current early returns when packages.Load fails, when
len(pkgs) == 0, or when any pkg.Errors exist remove the intended filesystem
fallback; change these branches to invoke the filesystem fallback (call
listGoSourceFilesFromFilesystem or the equivalent ListGoSourceFiles path)
instead of returning nil so discovery continues when package loading is
partial/non-module. Specifically, in the block around packages.Load and the loop
over pkgs check for errors or empty pkg list and call
listGoSourceFilesFromFilesystem(...) to collect files, and only return nil if
both package-based discovery and the filesystem fallback fail.
- Around line 103-105: The code silently ignores os.ReadFile errors (rawContent,
err := os.ReadFile(absPath) followed by continue), which swallows
permission/race failures; change this to propagate or log the error instead of
continuing: replace the bare continue with explicit error handling that wraps
and returns the error (e.g., return fmt.Errorf("read %s: %w", absPath, err)) or,
if the function must continue, emit a structured log including absPath and err
so failures are visible; update the function that contains this read (look for
the reader that uses absPath/rawContent) to propagate the returned error up the
call chain or record it via the repository logger.
In `@internal/gosourcefile/gosourcefile.go`:
- Around line 28-29: The Parse function currently uses types.Config with
importer.Default() and conf.Check (the conf.Check call in gosourcefile.Parse)
which only builds single-file GOPATH-style type info; change Parse to use the
packages loader (like internal/fsrepository/fsrepository.go) by creating a
packages.Config with Mode including packages.NeedTypesInfo and
packages.NeedSyntax (and any other modes needed), call packages.Load for the
file's package/module context, locate the loaded package that contains the
target AST file, and then extract the package-level TypesInfo and AST from that
result instead of using conf.Check so module-aware type resolution and
package-level info are produced.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: e48375b3-e45a-47c0-bcf0-c310af9d4425
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (10)
go.modinternal/fsrepository/fsrepository.gointernal/fsrepository/fsrepository_test.gointernal/gosourcefile/gosourcefile.gointernal/ignoredrepository/ignoredrepository_test.gointernal/oozetesting/fakerepository/fakerepository.gointernal/verboserepository/verboserepository_test.gooozetesting/test.gorelease.goviruses/cancelnil/cancelnil_test.go
💤 Files with no reviewable changes (1)
- viruses/cancelnil/cancelnil_test.go
Use golang.org/x/tools/go/packages to load source files with full type info, enabling viruses to make type-aware mutation decisions. Falls back to filesystem walk when package loading fails (e.g. no go.mod). - Enables the cancelnil virus which requires type info - Creates single path for GoSourceFile creation via gosourcefile.Parse() - Adds Equal and EqualSlice helpers for test comparisons - Removes unused reflect imports from test files Fixes #9 - resolves panic when running mutation tests against projects with internal packages by using module-aware loader instead of importer.Default().
430ea7a to
1f98e73
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
internal/fsrepository/fsrepository.go (1)
52-61:⚠️ Potential issue | 🟠 MajorMissing fallback to filesystem walk contradicts PR objectives.
The PR description states the implementation "Falls back to a filesystem walk when package loading fails (e.g., no go.mod)." However, the current code returns
nilwhenpackages.Loadfails, returns no packages, or when any package has errors. This will cause mutation testing to find zero source files in non-module repositories or when packages have type errors, rather than gracefully degrading.♻️ Proposed fix to add filesystem fallback
pkgs, err := packages.Load(cfg, "./...") if err != nil || len(pkgs) == 0 { - return nil + return r.listGoSourceFilesFromFilesystem() } for _, pkg := range pkgs { if len(pkg.Errors) > 0 { - return nil + return r.listGoSourceFilesFromFilesystem() } }Where
listGoSourceFilesFromFilesystem()implements the originalfilepath.WalkDirapproach withgosourcefile.Parse()for each discovered file.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/fsrepository/fsrepository.go` around lines 52 - 61, The current early returns after packages.Load and when pkg.Errors exist should be replaced with a graceful fallback to the filesystem walker: if packages.Load(cfg, "./...") returns an error, returns zero packages, or if any pkg in pkgs has non-empty Errors, call the existing listGoSourceFilesFromFilesystem() (or implement it if missing) to enumerate .go files and parse them with gosourcefile.Parse(), and return its result instead of nil; keep using packages.Load only when it succeeds with no package errors, and only return nil if both package loading and the filesystem fallback fail.
🤖 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.mod`:
- Line 17: Update the dependency line for golang.org/x/tools in go.mod from
v0.22.0 to a current stable version (e.g., v0.44.0), then run the Go module
update commands to fetch and reconcile transitive dependencies (ensure go.sum is
updated); specifically change the module version token "golang.org/x/tools
v0.22.0" to the newer version and run your usual go module sync (go get ./...
and go mod tidy) to verify builds and tests pass.
In `@internal/fsrepository/fsrepository.go`:
- Around line 103-106: The current loop silently skips files when
os.ReadFile(absPath) returns an error (rawContent, err := os.ReadFile(absPath) {
if err != nil { continue } }), which can hide permission or missing-file issues;
update this error branch to emit a debug/verbose log that includes absPath and
the err (preserving the continue behavior) so callers can diagnose missing
mutations—use the repository's existing logging facility or a debug flag to gate
the message and include clear context like "failed to read file" plus the path
and error.
---
Duplicate comments:
In `@internal/fsrepository/fsrepository.go`:
- Around line 52-61: The current early returns after packages.Load and when
pkg.Errors exist should be replaced with a graceful fallback to the filesystem
walker: if packages.Load(cfg, "./...") returns an error, returns zero packages,
or if any pkg in pkgs has non-empty Errors, call the existing
listGoSourceFilesFromFilesystem() (or implement it if missing) to enumerate .go
files and parse them with gosourcefile.Parse(), and return its result instead of
nil; keep using packages.Load only when it succeeds with no package errors, and
only return nil if both package loading and the filesystem fallback fail.
🪄 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: ASSERTIVE
Plan: Pro Plus
Run ID: 5a40c969-fe14-4c65-b01d-9e0c7daf6443
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (10)
go.modinternal/fsrepository/fsrepository.gointernal/fsrepository/fsrepository_test.gointernal/gosourcefile/gosourcefile.gointernal/ignoredrepository/ignoredrepository_test.gointernal/oozetesting/fakerepository/fakerepository.gointernal/verboserepository/verboserepository_test.gooozetesting/test.gorelease.goviruses/cancelnil/cancelnil_test.go
💤 Files with no reviewable changes (1)
- viruses/cancelnil/cancelnil_test.go
| github.com/fatih/color v1.18.0 | ||
| github.com/hexops/gotextdiff v1.0.3 | ||
| github.com/stretchr/testify v1.11.1 | ||
| golang.org/x/tools v0.22.0 |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check latest golang.org/x/tools version and security advisories
# Check for security advisories related to golang.org/x/tools
gh api graphql -f query='
{
securityVulnerabilities(first: 5, ecosystem: GO, package: "golang.org/x/tools") {
nodes {
advisory {
summary
severity
publishedAt
}
vulnerableVersionRange
firstPatchedVersion {
identifier
}
}
}
}'Repository: gtramontina/ooze
Length of output: 109
🏁 Script executed:
#!/bin/bash
# Verify golang.org/x/tools v0.22.0 is a valid release
# and check for version information
# Use Go to check module versions (requires go command)
# First, check go.mod file to see current state
cat go.mod | grep -A 2 "golang.org/x/tools"
# Try to fetch version info via web (golang.org serves module info)
curl -s "https://proxy.golang.org/golang.org/x/tools/@v/v0.22.0.info" 2>/dev/null || echo "Could not fetch from proxy"
# Also check latest version
curl -s "https://proxy.golang.org/golang.org/x/tools/@latest" 2>/dev/null || echo "Could not fetch latest"Repository: gtramontina/ooze
Length of output: 475
Update golang.org/x/tools to a current version.
The dependency has no known security vulnerabilities, but v0.22.0 (released June 2024) is significantly outdated. Version v0.44.0 is the current stable release (April 2026). Update to the latest or a more recent stable version to ensure access to bug fixes and improvements.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@go.mod` at line 17, Update the dependency line for golang.org/x/tools in
go.mod from v0.22.0 to a current stable version (e.g., v0.44.0), then run the Go
module update commands to fetch and reconcile transitive dependencies (ensure
go.sum is updated); specifically change the module version token
"golang.org/x/tools v0.22.0" to the newer version and run your usual go module
sync (go get ./... and go mod tidy) to verify builds and tests pass.
| rawContent, err := os.ReadFile(absPath) | ||
| if err != nil { | ||
| continue | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Silent error handling may mask file access issues.
If os.ReadFile fails (e.g., permissions, file deleted between load and read), the file is silently skipped. Consider logging this in verbose/debug mode to aid troubleshooting, especially since this could cause mutations to be silently missed.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/fsrepository/fsrepository.go` around lines 103 - 106, The current
loop silently skips files when os.ReadFile(absPath) returns an error
(rawContent, err := os.ReadFile(absPath) { if err != nil { continue } }), which
can hide permission or missing-file issues; update this error branch to emit a
debug/verbose log that includes absPath and the err (preserving the continue
behavior) so callers can diagnose missing mutations—use the repository's
existing logging facility or a debug flag to gate the message and include clear
context like "failed to read file" plus the path and error.
1 participant
Use
golang.org/x/tools/go/packagesto load source files with full typeinfo, enabling viruses to make type-aware mutation decisions. Falls back
to filesystem walk when package loading fails (e.g. no go.mod).
Fixes #9 - resolves panic when running mutation tests against projects
with internal packages by using module-aware loader instead of
importer.Default().