feat: repo-aware context — tree-sitter symbol extraction + grep usage finder - #20
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds repository-aware context discovery using Tree-sitter and grep, then appends related unchanged-code snippets to review prompts. Context discovery is enabled by default and can be disabled with ChangesRepository context pipeline
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant CLI
participant Reviewer
participant DefaultProvider
participant TreeSitterExtractor
participant GrepFinder
participant Model
CLI->>Reviewer: construct with context provider
Reviewer->>DefaultProvider: find related code for diff
DefaultProvider->>TreeSitterExtractor: extract changed symbols
DefaultProvider->>GrepFinder: find usages in repository
GrepFinder-->>DefaultProvider: return code snippets
DefaultProvider-->>Reviewer: return related snippets
Reviewer->>Model: build prompt with context
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.github.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (6)
internal/context/queries/typescript.scm (1)
2-3: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winConsider covering arrow-function exports.
Modern TS/React code frequently exports functions as
const foo = () => {}(hooks, handlers, utilities). These arevariable_declaratornodes with anarrow_functionvalue, notfunction_declaration, so they're currently invisible to this extractor.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/context/queries/typescript.scm` around lines 2 - 3, Extend the TypeScript query beyond function_declaration to match variable_declarator nodes whose value is an arrow_function, ensuring exported const-defined hooks, handlers, and utilities are captured as function symbols alongside existing declarations.internal/context/extractor.go (2)
16-18: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Extractdeclareserrorbut never returns one.
SymbolExtractor.Extractcommits callers to handling anerror, yet every internal failure (unreadable file, unknown grammar, bad query, parse failure) is logged atslog.Debugand swallowed; the function unconditionally returnsnilat line 89. This makes the error return misleading — callers cannot distinguish "nothing changed" from "extraction is silently broken everywhere."As per path instructions, "internal/**: Focus on error handling, context propagation, and interface design."
Also applies to: 38-90
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/context/extractor.go` around lines 16 - 18, Update the SymbolExtractor.Extract interface and its implementation to return meaningful extraction errors instead of declaring an error that is always nil. Propagate failures from unreadable files, unknown grammars, invalid queries, and parse errors to callers; preserve successful extraction results and allow genuinely empty changes to return without error.Source: Path instructions
16-18: 🩺 Stability & Availability | 🔵 Trivial | ⚖️ Poor tradeoffNo
context.Contextfor cancellable/bounded extraction.
Extractperforms repo-wide multi-file disk I/O and tree-sitter parsing with nocontext.Context, so a caller (e.g., a review pipeline with an overall timeout) has no way to cancel or bound this work if the diff touches many files.As per path instructions, "internal/**: Focus on error handling, context propagation, and interface design."
Also applies to: 38-38
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/context/extractor.go` around lines 16 - 18, Update the SymbolExtractor.Extract interface to accept a context.Context parameter and propagate it through every implementation and caller. Ensure repository file I/O and tree-sitter extraction honor cancellation promptly, returning the context error when cancellation or deadline expiry occurs.Source: Path instructions
internal/context/queries.go (1)
15-32: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winLog embed/read failures instead of swallowing silently.
If
queryFS.ReadDir/ReadFileever fail (e.g. an embed path mismatch),languageQueriesstays empty/partial and the whole context feature silently goes dark with no diagnostic trail, unlikeextractor.go's consistentslog.Debugon skip paths.As per path instructions, "internal/**: Focus on error handling, context propagation, and interface design."
♻️ Proposed fix
func init() { languageQueries = make(map[string]string) entries, err := queryFS.ReadDir("queries") if err != nil { + slog.Error("context: failed to read embedded queries dir", "error", err) return } for _, e := range entries { if e.IsDir() || !strings.HasSuffix(e.Name(), ".scm") { continue } data, err := queryFS.ReadFile("queries/" + e.Name()) if err != nil { + slog.Error("context: failed to read embedded query file", "file", e.Name(), "error", err) continue }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/context/queries.go` around lines 15 - 32, Update the init function’s queryFS.ReadDir and queryFS.ReadFile failure paths to emit slog.Debug diagnostics before returning or continuing, including the relevant error and operation/path context. Preserve the existing behavior of skipping unavailable entries while ensuring embed and read failures are no longer silent, consistent with extractor.go’s skip-path logging.Source: Path instructions
internal/context/context_test.go (1)
351-364: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
TestBuildUserPromptWithContextdoesn't test integration.This test only asserts on a hardcoded local
[]CodeSnippetliteral against itself; it never callsinternal/model's prompt-building code, so it can't catch a real integration regression despite the comment claiming otherwise. It will pass unconditionally forever. If real integration coverage lives ininternal/model's own tests (likely, since an internal test file here can't importmodelwithout a cycle), consider removing this placeholder or renaming/commenting it to make clear it's not asserting real behavior, so it isn't mistaken for coverage during future refactors.As per path instructions, "
**/*_test.go: Verify table-driven tests, proper cleanup, and race condition safety."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/context/context_test.go` around lines 351 - 364, The test TestBuildUserPromptWithContext is a placeholder that only validates a locally constructed CodeSnippet slice and does not exercise prompt-building integration. Remove this misleading test, or rename and revise its comments to clearly identify it as a local data-shape sanity check rather than integration coverage; keep real prompt-building coverage in the appropriate model tests.Source: Path instructions
internal/reviewer/reviewer.go (1)
141-162: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winContext discovery runs sequentially on the request hot path with unbounded symbol count.
FindRelatedCodeis invoked synchronously before any model call, and internallyGrepFinder.FindUsages(seegrep_finder.go) loops over every extracted symbol, launching a subprocess with up to a 5s timeout for each, before checking the total-snippet cap. For diffs that introduce many changed symbols, this can add many seconds of latency toRunbefore the first model request is even sent, on top of the fact that the whole discovery step is skipped only via a caught error rather than a bounded time budget.Consider passing a bounded overall timeout (e.g.,
context.WithTimeoutaround theFindRelatedCodecall) so a pathological diff can't stall the whole review indefinitely, complementing the per-symbol timeout already ingrep_finder.go.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/reviewer/reviewer.go` around lines 141 - 162, Bound the context-discovery phase in Run by creating a child context with a finite overall timeout before invoking r.contextProvider.FindRelatedCode. Use the bounded context for that call, ensure its cancellation is released, and preserve the existing warning-and-continue behavior when the timeout or another discovery error occurs.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/context/extractor.go`:
- Around line 57-64: Validate the resolved fullPath in the file-extraction loop
before calling os.ReadFile, ensuring it remains contained within repoRoot after
filepath cleaning and rejecting paths that escape via ../ segments. Preserve the
existing debug logging and skip behavior for rejected paths.
In `@internal/context/grep_finder.go`:
- Around line 166-190: Make the “too common” check independent of the snippet
limit in the symbol-matching flow around fileMatchCount and
MaxSnippetsPerSymbol. Count distinct matching files across the complete grep
result before applying the snippet cap, then return nil when the count exceeds
MaxFileMatches while retaining the existing capped snippet collection for
acceptable symbols.
- Around line 145-192: Fix grep result path handling in the symbol-matching
flow: update buildGrepArgs to search "." while retaining repoRoot as the command
working directory, so rg/grep emits relative paths compatible with diffFiles. In
grepSymbol, remove the broken filepath.Rel computation and use the parsed file
path directly for relPath and diff-file exclusion; clean up any now-unused
filepath import.
---
Nitpick comments:
In `@internal/context/context_test.go`:
- Around line 351-364: The test TestBuildUserPromptWithContext is a placeholder
that only validates a locally constructed CodeSnippet slice and does not
exercise prompt-building integration. Remove this misleading test, or rename and
revise its comments to clearly identify it as a local data-shape sanity check
rather than integration coverage; keep real prompt-building coverage in the
appropriate model tests.
In `@internal/context/extractor.go`:
- Around line 16-18: Update the SymbolExtractor.Extract interface and its
implementation to return meaningful extraction errors instead of declaring an
error that is always nil. Propagate failures from unreadable files, unknown
grammars, invalid queries, and parse errors to callers; preserve successful
extraction results and allow genuinely empty changes to return without error.
- Around line 16-18: Update the SymbolExtractor.Extract interface to accept a
context.Context parameter and propagate it through every implementation and
caller. Ensure repository file I/O and tree-sitter extraction honor cancellation
promptly, returning the context error when cancellation or deadline expiry
occurs.
In `@internal/context/queries.go`:
- Around line 15-32: Update the init function’s queryFS.ReadDir and
queryFS.ReadFile failure paths to emit slog.Debug diagnostics before returning
or continuing, including the relevant error and operation/path context. Preserve
the existing behavior of skipping unavailable entries while ensuring embed and
read failures are no longer silent, consistent with extractor.go’s skip-path
logging.
In `@internal/context/queries/typescript.scm`:
- Around line 2-3: Extend the TypeScript query beyond function_declaration to
match variable_declarator nodes whose value is an arrow_function, ensuring
exported const-defined hooks, handlers, and utilities are captured as function
symbols alongside existing declarations.
In `@internal/reviewer/reviewer.go`:
- Around line 141-162: Bound the context-discovery phase in Run by creating a
child context with a finite overall timeout before invoking
r.contextProvider.FindRelatedCode. Use the bounded context for that call, ensure
its cancellation is released, and preserve the existing warning-and-continue
behavior when the timeout or another discovery error occurs.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 9cec8121-82cf-496f-ad61-573e699d17c1
⛔ Files ignored due to path filters (2)
go.modis excluded by!go.modgo.sumis excluded by!**/*.sum,!go.sum
📒 Files selected for processing (15)
cmd/code-reviewer/main.gointernal/config/config.gointernal/context/context_test.gointernal/context/extractor.gointernal/context/grep_finder.gointernal/context/provider.gointernal/context/queries.gointernal/context/queries/go.scminternal/context/queries/java.scminternal/context/queries/kotlin.scminternal/context/queries/python.scminternal/context/queries/typescript.scminternal/context/types.gointernal/model/prompt.gointernal/reviewer/reviewer.go
Add ContextProvider that extracts changed symbols from diffs using tree-sitter (pure Go via gotreesitter, no CGo), then searches the repo for usages of those symbols in unchanged files. This gives the LLM visibility into code that may be broken by the diff, catching cross-file regressions like changed function signatures that break callers not included in the diff. Language support: Go, Kotlin, Java, Python, TypeScript. Architecture: SymbolExtractor (tree-sitter) → UsageFinder (grep) → CodeSnippets injected into user prompt as 'Related Unchanged Code' New package: internal/context/ - types.go: SymbolChange, CodeSnippet - provider.go: Provider interface, DefaultProvider - extractor.go: TreeSitterExtractor (parses files, filters to changed lines) - queries.go: embed.FS loader for .scm query files - queries/*.scm: tree-sitter queries for 5 languages - grep_finder.go: GrepFinder (ripgrep preferred, grep fallback) - context_test.go: 21 tests, 85% coverage Modified: - config.go: DisableContext + --no-context flag - prompt.go: BuildUserPromptWithContext, ContextSnippet type - prompt_test.go: 3 new tests for BuildUserPromptWithContext - reviewer.go: context discovery step, NewWithContext constructor - main.go: wire context provider - README.md: document REVIEW.md, repo-aware context, new flags - ROADMAP.md: mark REVIEW.md and context as done Noise mitigation: - MinNameLength (default 4) filters short symbols - MaxFileMatches (default 20) skips over-common symbols - isNoiseMatch filters imports and comments - MaxSnippetsPerSymbol (10) + MaxTotalSnippets (50) cap output
cb5c9af to
7b88786
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
internal/context/extractor.go (1)
57-64: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winPath traversal:
fullPathisn't validated to stay insiderepoRoot.
filepath.Join(repoRoot, path)doesn't preventpathfrom containing../segments that escape the repo checkout beforeos.ReadFile. Sincepathcomes from diff data (fd.NewPath), a maliciously crafted diff could read arbitrary files on disk, which then leak into generated review context/prompts.🔒 Suggested containment check
// Read the current file from disk. fullPath := filepath.Join(repoRoot, path) + cleanRoot := filepath.Clean(repoRoot) + if rel, err := filepath.Rel(cleanRoot, fullPath); err != nil || strings.HasPrefix(rel, "..") { + slog.Debug("context: rejected path escaping repoRoot", "path", path) + continue + } source, err := os.ReadFile(fullPath)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/context/extractor.go` around lines 57 - 64, Validate fullPath after joining it with repoRoot and before os.ReadFile, ensuring the resolved path remains within the resolved repository root; skip the entry with the existing debug behavior when traversal escapes the repository. Apply this in the file-extraction flow surrounding fullPath and preserve normal reads for contained fd.NewPath values.
🧹 Nitpick comments (5)
internal/context/extractor.go (1)
17-17: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffInconsistent context propagation across the package's interfaces.
Extractdoesn't accept acontext.Context, whileGrepFinder.FindUsages(ctx, ...)(used alongside it, perinternal/context/context_test.go) does. SinceExtractperforms file I/O per changed file in a loop, threading actxthrough would let callers cancel/timeout extraction consistently with the rest of the package's interface design.As per path instructions, "Focus on error handling, context propagation, and interface design" for
internal/**.Also applies to: 38-38
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/context/extractor.go` at line 17, Update the Extract interface to accept a context.Context and propagate it through the extraction workflow and each per-file I/O operation, matching GrepFinder.FindUsages. Update all callers, implementations, and related tests to pass the context while preserving existing extraction behavior and error handling.Source: Path instructions
internal/context/context_test.go (3)
351-364: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest doesn't exercise any integration — asserts on a locally-constructed literal.
TestBuildUserPromptWithContextbuilds a[]CodeSnippetand immediately asserts properties of that same literal; it never callsBuildUserPromptWithContextor any conversion/mapping function. The comment claims it verifies "the provider correctly maps types," but no mapping code path is invoked, so this test can't catch a regression in that mapping.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/context/context_test.go` around lines 351 - 364, Update TestBuildUserPromptWithContext to invoke BuildUserPromptWithContext with the test snippets and assert against the resulting prompt or mapped output. Remove assertions that only inspect the locally constructed snippets, and verify the provider mapping behavior through the actual integration path.
645-650: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSimplify test filename construction.
filepath.Clean(filepath.Join(".", "file"+string(rune('a'+i))+".go"))is a roundabout way to build "filea.go".."filee.go".🧹 Suggested simplification
- f := filepath.Join(repoRoot, filepath.Clean(filepath.Join(".", "file"+string(rune('a'+i))+".go"))) + f := filepath.Join(repoRoot, fmt.Sprintf("file%c.go", 'a'+i))🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/context/context_test.go` around lines 645 - 650, Simplify the test filename construction inside the range loop by directly building the filea.go through filee.go names, removing the unnecessary nested filepath.Join, filepath.Clean, and rune conversion while preserving the existing paths and file-writing behavior.
598-598: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove dead
_ = middlewareFileassignment.
middlewareFileis already used as theos.WriteFileargument on line 591; this blank assignment is a leftover debug artifact.🧹 Suggested cleanup
- _ = middlewareFile🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/context/context_test.go` at line 598, Remove the redundant blank assignment to middlewareFile in the test, leaving its existing use as the os.WriteFile argument unchanged.internal/context/queries/kotlin.scm (1)
1-9: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueLimited symbol coverage — functions/classes/objects only.
No queries for Kotlin properties, extension functions, secondary constructors, or companion objects, so changes to those won't surface "Related Unchanged Code" usages. Reasonable for an initial pass; consider expanding coverage in a follow-up.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/context/queries/kotlin.scm` around lines 1 - 9, Expand the Kotlin symbol extraction queries beyond function_declaration, class_declaration, and object_declaration to include properties, extension functions, secondary constructors, and companion objects. Add appropriate symbol captures for each construct while preserving the existing function, class, and object coverage.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@internal/context/extractor.go`:
- Around line 57-64: Validate fullPath after joining it with repoRoot and before
os.ReadFile, ensuring the resolved path remains within the resolved repository
root; skip the entry with the existing debug behavior when traversal escapes the
repository. Apply this in the file-extraction flow surrounding fullPath and
preserve normal reads for contained fd.NewPath values.
---
Nitpick comments:
In `@internal/context/context_test.go`:
- Around line 351-364: Update TestBuildUserPromptWithContext to invoke
BuildUserPromptWithContext with the test snippets and assert against the
resulting prompt or mapped output. Remove assertions that only inspect the
locally constructed snippets, and verify the provider mapping behavior through
the actual integration path.
- Around line 645-650: Simplify the test filename construction inside the range
loop by directly building the filea.go through filee.go names, removing the
unnecessary nested filepath.Join, filepath.Clean, and rune conversion while
preserving the existing paths and file-writing behavior.
- Line 598: Remove the redundant blank assignment to middlewareFile in the test,
leaving its existing use as the os.WriteFile argument unchanged.
In `@internal/context/extractor.go`:
- Line 17: Update the Extract interface to accept a context.Context and
propagate it through the extraction workflow and each per-file I/O operation,
matching GrepFinder.FindUsages. Update all callers, implementations, and related
tests to pass the context while preserving existing extraction behavior and
error handling.
In `@internal/context/queries/kotlin.scm`:
- Around line 1-9: Expand the Kotlin symbol extraction queries beyond
function_declaration, class_declaration, and object_declaration to include
properties, extension functions, secondary constructors, and companion objects.
Add appropriate symbol captures for each construct while preserving the existing
function, class, and object coverage.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 596e153f-462f-40c0-bfc4-ee955818a10f
⛔ Files ignored due to path filters (2)
go.modis excluded by!go.modgo.sumis excluded by!**/*.sum,!go.sum
📒 Files selected for processing (18)
README.mdROADMAP.mdcmd/code-reviewer/main.gointernal/config/config.gointernal/context/context_test.gointernal/context/extractor.gointernal/context/grep_finder.gointernal/context/provider.gointernal/context/queries.gointernal/context/queries/go.scminternal/context/queries/java.scminternal/context/queries/kotlin.scminternal/context/queries/python.scminternal/context/queries/typescript.scminternal/context/types.gointernal/model/prompt.gointernal/model/prompt_test.gointernal/reviewer/reviewer.go
🚧 Files skipped from review as they are similar to previous changes (12)
- internal/context/queries/java.scm
- internal/context/queries/python.scm
- internal/model/prompt.go
- internal/context/types.go
- internal/context/queries.go
- cmd/code-reviewer/main.go
- internal/context/queries/typescript.scm
- internal/config/config.go
- internal/context/queries/go.scm
- internal/context/provider.go
- internal/reviewer/reviewer.go
- internal/context/grep_finder.go
Security:
- extractor.go: Guard against path traversal via filepath.Abs +
strings.HasPrefix before os.ReadFile. Rejects ../../ paths.
- Add TestTreeSitterExtractor_PathTraversal test.
Bug fix:
- grep_finder.go: Restructure parseGrepOutput to count ALL unique
files from raw grep output before collecting snippets. Previously
the frequency cap (MaxFileMatches=20) was dead code because the
loop broke at MaxSnippetsPerSymbol=10 first. Now uses two-pass:
1) scan all lines → count unique files → bail if too common
2) collect up to MaxSnippetsPerSymbol snippets
Cleanup:
- Remove dead '_ = middlewareFile' assignment
- Simplify test filename construction with fmt.Sprintf
- Fix Kotlin .scm query (removed invalid interface_declaration)
- Fix TypeScript .scm query (class/interface use type_identifier)
There was a problem hiding this comment.
♻️ Duplicate comments (1)
internal/context/extractor.go (1)
61-67: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winThe containment check still allows symlink escapes.
filepath.Abscleans lexical..segments but does not resolve symlinks. A path such aslink/secret.go, wherelinkpoints outsiderepoRoot, can pass this prefix check whileos.ReadFilefollows the link and reads external content. Resolve both paths withfilepath.EvalSymlinks, compare them usingfilepath.Rel, handle resolution errors instead of discarding them, and add a symlink-escape test. This extends the previous path-traversal finding.#!/bin/bash set -euo pipefail rg -n 'filepath\.(Abs|EvalSymlinks|Rel)|os\.ReadFile|Symlink|PathTraversal' \ internal/context/extractor.go internal/context/context_test.goAs per path instructions: “Focus on error handling, context propagation, and interface design.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/context/extractor.go` around lines 61 - 67, The containment check in the context extraction flow must prevent symlink escapes. Update the path resolution around resolvedRoot and resolvedPath to use filepath.EvalSymlinks for both paths, handle and propagate or skip on resolution errors instead of discarding them, and compare containment with filepath.Rel rather than a string prefix. Add coverage in the existing context tests for a symlink pointing outside the repository.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@internal/context/extractor.go`:
- Around line 61-67: The containment check in the context extraction flow must
prevent symlink escapes. Update the path resolution around resolvedRoot and
resolvedPath to use filepath.EvalSymlinks for both paths, handle and propagate
or skip on resolution errors instead of discarding them, and compare containment
with filepath.Rel rather than a string prefix. Add coverage in the existing
context tests for a symlink pointing outside the repository.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 4c19b1c5-0af3-45f4-a4f8-0248770c33a6
📒 Files selected for processing (3)
internal/context/context_test.gointernal/context/extractor.gointernal/context/grep_finder.go
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/context/grep_finder.go
- internal/context/context_test.go
Use filepath.EvalSymlinks instead of filepath.Abs to resolve both '..' traversal AND symlinks to their real paths before the containment check. Errors during resolution (broken symlinks, etc.) cause the file to be skipped gracefully. Add TestTreeSitterExtractor_SymlinkEscape covering a symlink inside the repo pointing to a file outside it.
Shipped: - LLM proxy support (--proxy-url) - VCS interface abstraction (internal/vcs) - REVIEW.md repo-level instructions (PR #19) - Repo-aware context via tree-sitter + grep (PR #20) Reorganize roadmap: - v0.5: Platform Expansion (GitHub support, Actions, auto-approve) - v0.6: Deep Intelligence (multi-pass, RAG, import-aware)
1 participant
Summary
Adds a ContextProvider that gives the LLM visibility into unchanged code that may be broken by the diff. This catches cross-file regressions like changed function signatures breaking callers not included in the diff.
The Problem
If
auth.gochangesValidateSession(token string)→ValidateSession(token string, opts ...Option), the reviewer has no way to know thathandler.go(not in the diff) callsValidateSession(token)and is now broken.The Solution
Related Unchanged CodeLanguage Support
Key Design Decisions
gotreesitter): No CGo, no C toolchain. Clean cross-compilation.--no-contextflag to disable.New Package:
internal/context/types.goSymbolChange,CodeSnippettypesprovider.goProviderinterface,DefaultProviderextractor.goTreeSitterExtractor— parse + filter to changed linesqueries.goembed.FSloader for .scm query filesqueries/*.scmgrep_finder.goGrepFinder— ripgrep/grep usage searchcontext_test.goModified Files
config.goDisableContext+--no-contextflagprompt.goBuildUserPromptWithContext(),ContextSnippettypereviewer.goNewWithContext()constructormain.goExample Output
When reviewing a diff that changes
ValidateSession, the user prompt now includes:Summary by CodeRabbit
REVIEW.mdas a high-priority input.--no-contextto disable related-code discovery.