Conversation
…links and stop the program rather than hanging indefinitely
…ink loops, and skips instead of running indefinitely (Michael)
… on Windows machines, because Windows requires administrator mode or developer mode to be enabled to create symlinks.
# Conflicts: # processor/file_test.go
…ink loops, and skips instead of running indefinitely (Michael) this is a recommit, the other commit by Richard, possibly might have overwritten it? overall very weird that both our code dissapered
… on Windows machines, because Windows requires administrator mode or developer mode to be enabled to create symlinks.
The function TestNewFileJobDuplicateCounting checks that the same file is not processed twice when accessed through multiple paths with symLink. This should skip duplicate files and prevents double counting.
…k for fist link created in my test, and used BrokenSymlink check window check in the Duplicate test
… the complexity/lines
…ty/Lines is calculating the collective complexity divide by the total code and then multiply it by 100 to get a percent form.
…ty/Lines is calculating the collective complexity divide by the total code and then multiply it by 100 to get a percent form.
pr-insights
Bot
added
M/complexity
There was a problem hiding this comment.
Pull request overview
Fixes incorrect Complexity/Lines aggregation in --wide output by recalculating it from total complexity and total code lines (instead of summing per-file ratios). The PR also introduces symlink de-duplication/loop avoidance behavior in file job creation, with accompanying tests.
Changes:
- Recompute language and total
Complexity/Linesusing aggregatedComplexityandCodesums infileSummarizeLong. - Add a regression test validating correct
Complexity/Linesbehavior in wide output. - Add symlink handling changes (visited-path tracking) and new symlink-focused tests; update Go module metadata.
Reviewed changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| processor/formatters.go | Fixes wide summary Complexity/Lines to be computed from totals (language + overall). |
| processor/formatters_test.go | Adds regression test for wide Complexity/Lines aggregation. |
| processor/file.go | Adds global visited-path tracking to avoid duplicate processing/loops (symlink-related). |
| processor/file_test.go | Adds tests for broken/circular symlinks and duplicate counting via symlinks. |
| go.mod | Promotes github.com/mark3labs/mcp-go to a direct dependency. |
| go.sum | Adds/updates transitive dependency checksums. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Added as a way to track files per run. | ||
| var visitedPaths sync.Map | ||
|
|
There was a problem hiding this comment.
visitedPaths is a package-level global but it’s never reset as part of a run. That means repeated calls to Process()/library entrypoints in the same process (and the existing unit tests that call newFileJob across multiple tests) will start skipping files as “already processed”. Consider resetting this at the beginning of each run (e.g., in Process()/ProcessConstants()) or moving it into a per-run context instead of a global.
| // Added as a way to track files per run. | |
| var visitedPaths sync.Map | |
| // Added as a way to track files per run. | |
| // This must be reset at the start of each processing run so state does not | |
| // leak across repeated library calls or tests executed in the same process. | |
| var visitedPaths sync.Map | |
| func resetVisitedPaths() { | |
| visitedPaths = sync.Map{} | |
| } |
| t.Fatal("Failed to create broken symlink:", err) | ||
| } | ||
|
|
||
| fi, _ := os.Lstat(symPath) |
There was a problem hiding this comment.
Error from os.Lstat(symPath) is ignored. If Lstat fails, fi will be nil and newFileJob will panic when dereferencing fileInfo. Check the error and t.Fatal on failure.
| fi, _ := os.Lstat(symPath) | |
| fi, err := os.Lstat(symPath) | |
| if err != nil { | |
| t.Fatal("Failed to lstat broken symlink:", err) | |
| } |
| fi1, _ := os.Lstat(testFile) | ||
| job1 := newFileJob(testFile, "file.go", fi1) | ||
|
|
||
| // Process the symlink (same target) | ||
| fi2, _ := os.Lstat(linkFile) |
There was a problem hiding this comment.
Errors from os.Lstat are ignored when creating fi1/fi2. If either fails, newFileJob can panic due to a nil fileInfo. Handle both errors explicitly (and fail the test) so failures are actionable.
| fi1, _ := os.Lstat(testFile) | |
| job1 := newFileJob(testFile, "file.go", fi1) | |
| // Process the symlink (same target) | |
| fi2, _ := os.Lstat(linkFile) | |
| fi1, err := os.Lstat(testFile) | |
| if err != nil { | |
| t.Fatalf("os.Lstat(%q) failed: %v", testFile, err) | |
| } | |
| job1 := newFileJob(testFile, "file.go", fi1) | |
| // Process the symlink (same target) | |
| fi2, err := os.Lstat(linkFile) | |
| if err != nil { | |
| t.Fatalf("os.Lstat(%q) failed: %v", linkFile, err) | |
| } |
| Files = true | ||
| res := fileSummarizeLong(inputChan) | ||
| Files = false |
There was a problem hiding this comment.
This test mutates the global Files flag. To avoid leaking state if the test fails/panics before resetting, set the original value aside and restore it via t.Cleanup/defer.
| Files = true | |
| res := fileSummarizeLong(inputChan) | |
| Files = false | |
| originalFiles := Files | |
| t.Cleanup(func() { | |
| Files = originalFiles | |
| }) | |
| Files = true | |
| res := fileSummarizeLong(inputChan) |
| // This determines the real path | ||
| realPath := path | ||
| if symPath != "" { | ||
| realPath = symPath | ||
| } |
There was a problem hiding this comment.
The duplicate-detection key uses path/symPath directly. However, newFileJob is called in this repo with path sometimes being a directory (and name the filename), which will cause every file in that directory to be treated as a duplicate and skipped. Use a stable key that always represents the actual file path being processed (e.g., if path != name and filepath.Base(path) != name then filepath.Join(path, name), otherwise path; still prefer the resolved symPath when present).
| if _, exists := visitedPaths.Load(realPath); exists { | ||
| printWarnF("skipping already processed file: %s", realPath) | ||
| return nil | ||
| } | ||
| visitedPaths.Store(realPath, true) |
There was a problem hiding this comment.
The Load then Store sequence on visitedPaths is not atomic. If newFileJob is ever called concurrently (the code already uses goroutines heavily), two goroutines can both observe “not visited” and both process the same file. Use visitedPaths.LoadOrStore(realPath, true) to make the check+set atomic.
| if _, exists := visitedPaths.Load(realPath); exists { | |
| printWarnF("skipping already processed file: %s", realPath) | |
| return nil | |
| } | |
| visitedPaths.Store(realPath, true) | |
| if _, loaded := visitedPaths.LoadOrStore(realPath, true); loaded { | |
| printWarnF("skipping already processed file: %s", realPath) | |
| return nil | |
| } |
| // Added as a way to track files per run. | ||
| var visitedPaths sync.Map |
There was a problem hiding this comment.
PR description focuses on fixing Complexity/Lines aggregation, but this change set also introduces new global symlink de-duplication behavior via visitedPaths. Please update the PR description (or split into a separate PR) so reviewers/users understand the additional behavior change and its rationale.
| ProcessConstants() | ||
| IncludeSymLinks = true | ||
|
|
There was a problem hiding this comment.
IncludeSymLinks is a global; this test sets it to true and only resets it at the end. If the test fails early (e.g., t.Fatal) it can leak state into other tests. Use t.Cleanup/defer to restore IncludeSymLinks immediately after setting it.
|
Some commits are duplicated for #694. Only these 3 are new: 
Maybe the branch should be updated. @vrajpatel-bsu @boyter |
Issue: As of this status report scc issue #412 has been fixed. The issue originally was brought up by another user and is listed as issue #370. Both of these issues identify a problem with how Complexity/Lines is calculated when using a parameter called –wide. What should happen when calculating Complexity/Lines is the total complexity and total code lines between all files should be summed in their respective categories and then the Complexity/Lines equation should be used to determine the total complexity of the project. For example Files 1, 2, and 3 have complexity of 10 and code lines of 20. In total the project will have a complexity of 30 and code lines of 60. When the equation is implemented the result should be 50, but it comes out as 150.
Solution: As a solution to this I changed the parameters for Complexity/Lines from summing each of the file’s Complexity/Lines to recalculating the Complexity/Lines from the total complexity and total code lines.
Test: As a test case for this I created 3 temp files which will have place holder values for each category and make sure the expected total for the Complexity/Lines is returned. Else it will fail and return the entire table to trouble shoot.