fix(tool): prevent path traversal bypass and normalize paths in code_search and code_comment

[MeiSiristhebest]

Description

This PR fixes two cross-platform path handling issues in internal/tool:

1. Fix path traversal bypass & normalize pathspecs in code_search:

   • hasTraversalPathComponent now normalizes backslashes \ to / before splitting on /, preventing traversal sequences like ..\pkg or pkg\..\internal from bypassing the .. safety check.
   • Normalizes file_patterns items from \ to / before passing them to git grep so pathspec matching works consistently on all platforms.

2. Normalize comment paths in code_comment:

   • parseCommentsInner now canonicalizes comment paths (\ to /, stripping leading ./, resolving ., and trimming redundant slashes via normalizeCommentPath).
   • Ensures that review comments produced with Windows backslashes or leading ./ correctly match CommentCollector.CommentsForPath(d.NewPath) during review compilation instead of being silently dropped.

Type of Change

• Bug fix (non-breaking change that fixes an issue)
• New feature (non-breaking change that adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)
• Refactoring (no functional changes)
• Documentation update
• CI / Build / Tooling

How Has This Been Tested?

• Added TestParseComments_NormalizePath in internal/tool/code_comment_test.go to verify normalization of \, ./, and redundant slashes.

• Added backslash test cases to TestCodeSearchProvider_Execute_RejectsTraversalPattern in internal/tool/code_search_test.go.

• Verified targeted unit tests and full package tests pass cleanly (go test -v ./internal/tool/...).

• make test passes locally

• Unit tests pass locally

Checklist

• My code follows the project's coding style (go fmt, go vet)
• I have performed a self-review of my code
• I have added tests that prove my fix is effective or my feature works
• New and existing unit tests pass locally with my changes
• I have updated the documentation accordingly (if applicable)
• I have signed the CLA

Related Issues

Closes #1088

[github-actions]

✅ OpenCodeReview: Review complete: 0 finding(s) across 2 selected item(s).

[Qiyuanqiii]

good

[lizhengfeng101]

Thanks for the PR. The hardening in hasTraversalPathComponent makes sense as defense-in-depth, and the normalizeCommentPath logic is clean.

That said — I ran a quick scan across 174 historical review sessions on this repo (1580 code_search calls, 174 code_comment calls). Zero instances of backslash paths, ./ prefixes, redundant slashes, or .. in either tool. The LLM has never produced a malformed path in practice here.

A few questions:

1. Did you actually hit this in the wild (dropped comments, failed pathspec match), or is this purely from reading the code?
2. For code_search: git on Linux/macOS doesn't treat \ as a separator, so even if the check is bypassed, git grep -- '..\pkg' matches nothing real. The actual security boundary is git itself, not our string check. Do you have a scenario where this matters beyond the theoretical?
3. Minor: path.Clean resolves .. silently — an LLM hallucinating pkg/../secret.go gets normalized to secret.go rather than rejected. Intentional?

Not blocking — the code is correct and the tests are good. Just want to calibrate whether this addresses an observed issue or a hypothetical one.

[wu21-web]

You are missing coverage.

Suggested change

	func TestCodeSearchProvider_Execute_WithFilePatterns(t *testing.T) {
	dir := setupTestRepo(t)
	p := NewCodeSearch(&FileReader{RepoDir: dir, Mode: ModeWorkspace})
	tests := []struct {
	name string
	pattern string
	}{
	{name: "forward slash", pattern: "pkg/"},
	{name: "backslash", pattern: "pkg\\"},
	}
	for _, test := range tests {
	t.Run(test.name, func(t *testing.T) {
	got, err := p.Execute(context.Background(), map[string]any{
	"search_text": "Util",
	"file_patterns": []any{test.pattern},
	})
	if err != nil {
	t.Fatal(err)
	}
	if !strings.Contains(got, "util.go") {
	t.Errorf("expected util.go for pattern %q, got: %s", test.pattern, got)
	}
	})
	}
	}

[MeiSiristhebest]

Thanks for the feedback and discussion @lizhengfeng101!

To share what we actually observed testing locally in a native Windows environment:

While the CLI doesn't crash thanks to the existing fallbacks, path discrepancies cause subtle runtime friction on Windows:

1. Rule matching fallback: Passing Windows paths (e.g. via Tab-completion) causes doublestar.Match to miss language globs (**/*.ts), quietly falling back to system/default instead of language-specific rules.
2. Empty retries in llmloop: When backslash patterns in code_search hit Git pathspecs, searches return empty, causing llmloop to trigger consecutiveEmptyRounds retry turns. That ends up burning redundant LLM tokens and adding latency.
3. path.Clean: This was just meant as a forgiving reconciler for harmless relative paths so valid findings don't get dropped by CommentsForPath, while hard security and boundary checks remain at the tool perimeter.

Also, I have incorporated @wu21-web's test coverage suggestion in the latest commit (1c80093). Appreciate the great review and collaboration!