feat(agent): add agent mode for cross-document evaluation#74
Closed
oshorefueled wants to merge 34 commits intomainfrom
Closed
feat(agent): add agent mode for cross-document evaluation#74oshorefueled wants to merge 34 commits intomainfrom
oshorefueled wants to merge 34 commits intomainfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds an LLM-driven "agent" mode: new agent executor, workspace‑scoped tools (read/search/files/list/lint), Zod-validated agent finding types, CLI wiring for Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI
participant Orch as Orchestrator
participant Agent as Agent Executor
participant LLM as Language Model
participant Tools as Tool Set
participant FS as File System
CLI->>Orch: evaluateFiles(targets, { mode: "agent" })
Orch->>Orch: build tools and diffContext
Orch->>Orch: model = provider.getLanguageModel()
Orch->>Agent: runAgentExecutor({ rule, cwd, model, tools, diffContext })
Agent->>LLM: generateText(systemPrompt + toolSchemas, stepLimit)
loop Agent-driven tool calls
LLM->>Tools: invoke tool (read_file / search_content / list_directory / lint)
Tools->>Tools: resolve path & isWithinRoot check
Tools->>FS: read/list/search files
FS-->>Tools: content/results
Tools-->>LLM: tool response (paginated/truncated/no-match)
end
LLM-->>Agent: structured JSON { findings: [...] }
Agent->>Agent: validate with AGENT_FINDING_SCHEMA
Agent-->>Orch: { findings, ruleId, error? }
Orch->>Orch: collectAgentFindings(allResults)
Orch-->>CLI: print findings / emit JSON summary
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 6
🧹 Nitpick comments (3)
tests/agent/list-directory.test.ts (1)
39-46: Minor redundancy:subdiris already created inbeforeEach.Line 40 re-creates
subdirwhich is already set up bybeforeEachat line 9. Therecursive: truemakes this harmless, but you could simplify by only writing the nested file.♻️ Suggested simplification
it('lists a specific subdirectory', async () => { - mkdirSync(path.join(TMP, 'subdir'), { recursive: true }); writeFileSync(path.join(TMP, 'subdir', 'nested.md'), ''); const tool = createListDirectoryTool(TMP);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/agent/list-directory.test.ts` around lines 39 - 46, The test "lists a specific subdirectory" redundantly recreates the subdir; remove the mkdirSync(path.join(TMP, 'subdir'), { recursive: true }) call and keep only writeFileSync(path.join(TMP, 'subdir', 'nested.md'), '') so the test uses the setup from beforeEach; locate the test using the it block name and the createListDirectoryTool(TMP) / tool.execute({ path: 'subdir' }) calls to update the snippet.src/agent/tools/list-directory.ts (1)
18-59: Consider using async/await for consistency.The function returns
Promise<string>but wraps synchronous operations inPromise.resolve/Promise.reject. Other tools in this module (e.g.,search-files.ts) useasync/await. For consistency and readability, consider making this anasyncfunction.♻️ Proposed refactor using async/await
- execute({ path: dirPath, limit }) { - try { - const absolutePath = resolveToCwd(dirPath || '.', cwd); - - if (!isWithinRoot(absolutePath, cwd)) { - return Promise.reject(new Error(`Path traversal blocked: ${dirPath} is outside the allowed root`)); - } - - if (!existsSync(absolutePath)) { - return Promise.reject(new Error(`Directory not found: ${dirPath}`)); - } + async execute({ path: dirPath, limit }) { + const absolutePath = resolveToCwd(dirPath || '.', cwd); + + if (!isWithinRoot(absolutePath, cwd)) { + throw new Error(`Path traversal blocked: ${dirPath} is outside the allowed root`); + } + + if (!existsSync(absolutePath)) { + throw new Error(`Directory not found: ${dirPath}`); + } // ... rest of implementation using return instead of Promise.resolve - return Promise.resolve(output); - } catch (error) { - return Promise.reject(error instanceof Error ? error : new Error(String(error))); - } + return output; },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/agent/tools/list-directory.ts` around lines 18 - 59, Convert the execute method to an async function and stop wrapping sync results in Promise.resolve/Promise.reject: change the execute signature to async execute({ path: dirPath, limit }) and inside use the existing try/catch but return plain strings or throw Errors instead of Promise.resolve/Promise.reject; keep using resolveToCwd, isWithinRoot, existsSync, readdirSync, statSync and path.join as before, compute effectiveLimit from DEFAULT_LIMIT, build results, and when errors occur throw the Error (or rethrow in catch using throw error instanceof Error ? error : new Error(String(error))). Also remove any Promise.resolve/Promise.reject uses and adjust the truncation message logic to return the combined string directly.src/agent/agent-executor.ts (1)
140-154: Consider adding fallback parsing for structured output robustness.The Vercel AI SDK v6.x throws
NoOutputGeneratedErrorwhen structured output parsing fails (commonly whenfinishReasonis not "stop", especially with AI Gateway or certain provider configurations). While the existing try/catch handles the exception, the recommended pattern in the SDK's issue tracker is to catchNoOutputGeneratedErrorand fall back to manually parsingresult.textas JSON when available. This improves robustness without changing the happy path.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/agent/agent-executor.ts` around lines 140 - 154, Catch the specific NoOutputGeneratedError thrown by generateText and implement a fallback that attempts to parse the raw text output (e.g., result.text or response.outputText) as JSON when structured parsing fails; update the try/catch around generateText in agent-executor (where generateText, AGENT_OUTPUT_SCHEMA, and stepCountIs(MAX_AGENT_STEPS) are used) to detect NoOutputGeneratedError, attempt JSON.parse on the raw result text to extract findings and ruleId, and only rethrow if parsing is impossible so the existing happy path using response.output.findings remains unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/agent/tools/path-utils.ts`:
- Around line 17-30: The containment check in isWithinRoot is unsafe because
normalizePath can use realpathSync for one input and path.resolve for the other;
fix by applying the same normalization strategy to both paths: attempt
realpathSync on both and if either call throws, fall back to using path.resolve
for both so both normalizedRoot and normalizedPath are produced by the same
method; keep the final startsWith/equals check and ensure you still append
path.sep when checking prefix so path boundary logic (normalizedRoot + path.sep)
remains correct.
In `@src/agent/tools/search-content.ts`:
- Line 159: Update the tool description string (the description property in the
search-content tool definition) to accurately state the default glob filter as
"**/*.md" instead of "*.md"; verify consistency with the other occurrences that
use "**/*.md" (seen near the uses at lines referencing the default glob in this
file) so the description matches the actual default behavior.
- Around line 104-109: The code constructs a RegExp directly from the untrusted
pattern (pattern / new RegExp(...)) which allows ReDoS; fix by validating or
replacing the engine before compiling: either validate the pattern with a safety
check (e.g., integrate safe-regex to reject dangerous patterns) or switch to a
backtracking-free engine (e.g., use re2 to instantiate the regex instead of
RegExp) and/or wrap regex execution in a short timeout guard to abort
long-running matches; update the logic around the RegExp creation in
search-content.ts where regex = new RegExp(pattern, opts.ignoreCase ? 'i' : ''),
rejecting unsafe patterns (returning the same error string) or using re2 and
ensure opts.ignoreCase behavior is preserved.
In `@src/agent/tools/search-files.ts`:
- Around line 25-30: The fast-glob results in searchFiles (function in
src/agent/tools/search-files.ts) return paths relative to searchRoot when a
subdirectory `path` is provided, but caller tools (read_file, lint) expect
repo-relative paths, so prepend the provided `path` prefix to each match before
returning; import Node's `path` module, update the function description to state
it returns repository-relative paths, and ensure both the code branches that map
`matches` (the arrays created around lines with fg(...) and the later mapping at
36-42) add `path.join(pathPrefix, match)` (or similar) only when a non-empty
`path` argument was passed.
In `@src/cli/orchestrator.ts`:
- Around line 190-196: The RdJson/ValeJson branches instantiate RdJsonFormatter
and ValeJsonFormatter but never add agent findings, causing formatter.toJson()
to emit empty output; update the branches handling OutputFormat.RdJson and
OutputFormat.ValeJson in orchestrator.ts (the sections creating RdJsonFormatter
and ValeJsonFormatter) to either (A) log a warning that RdJson/ValeJson are not
supported in agent mode and fall back to the existing JSON formatter path (e.g.,
reuse the code that populates findings for JSON output), or (B) map the
collected agent findings into the RdJsonFormatter/ValeJsonFormatter APIs before
calling formatter.toJson(); implement one of these fixes and ensure the
warning/fallback is clearly emitted when in agent mode so users don’t get empty
output.
In `@src/output/reporter.ts`:
- Line 229: The ternary that computes loc uses a truthy check that treats 0 as
absent; update the expression that sets loc to check explicitly for undefined
(reference.startLine !== undefined) so a valid startLine of 0 is preserved—loc
should remain `${reference.file}:${reference.startLine}` when startLine is any
number, and fall back to reference.file only when startLine is strictly
undefined; modify the assignment that references reference.startLine in
src/output/reporter.ts accordingly.
---
Nitpick comments:
In `@src/agent/agent-executor.ts`:
- Around line 140-154: Catch the specific NoOutputGeneratedError thrown by
generateText and implement a fallback that attempts to parse the raw text output
(e.g., result.text or response.outputText) as JSON when structured parsing
fails; update the try/catch around generateText in agent-executor (where
generateText, AGENT_OUTPUT_SCHEMA, and stepCountIs(MAX_AGENT_STEPS) are used) to
detect NoOutputGeneratedError, attempt JSON.parse on the raw result text to
extract findings and ruleId, and only rethrow if parsing is impossible so the
existing happy path using response.output.findings remains unchanged.
In `@src/agent/tools/list-directory.ts`:
- Around line 18-59: Convert the execute method to an async function and stop
wrapping sync results in Promise.resolve/Promise.reject: change the execute
signature to async execute({ path: dirPath, limit }) and inside use the existing
try/catch but return plain strings or throw Errors instead of
Promise.resolve/Promise.reject; keep using resolveToCwd, isWithinRoot,
existsSync, readdirSync, statSync and path.join as before, compute
effectiveLimit from DEFAULT_LIMIT, build results, and when errors occur throw
the Error (or rethrow in catch using throw error instanceof Error ? error : new
Error(String(error))). Also remove any Promise.resolve/Promise.reject uses and
adjust the truncation message logic to return the combined string directly.
In `@tests/agent/list-directory.test.ts`:
- Around line 39-46: The test "lists a specific subdirectory" redundantly
recreates the subdir; remove the mkdirSync(path.join(TMP, 'subdir'), {
recursive: true }) call and keep only writeFileSync(path.join(TMP, 'subdir',
'nested.md'), '') so the test uses the setup from beforeEach; locate the test
using the it block name and the createListDirectoryTool(TMP) / tool.execute({
path: 'subdir' }) calls to update the snippet.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 00616295-063b-477b-b4e0-26cada6dd396
📒 Files selected for processing (28)
docs/logs/2026-03-17-agentic-capabilities.log.mdsrc/agent/agent-executor.tssrc/agent/index.tssrc/agent/merger.tssrc/agent/tools/index.tssrc/agent/tools/lint-tool.tssrc/agent/tools/list-directory.tssrc/agent/tools/path-utils.tssrc/agent/tools/read-file.tssrc/agent/tools/search-content.tssrc/agent/tools/search-files.tssrc/agent/types.tssrc/cli/commands.tssrc/cli/orchestrator.tssrc/cli/types.tssrc/output/json-formatter.tssrc/output/reporter.tssrc/providers/llm-provider.tssrc/providers/vercel-ai-provider.tssrc/schemas/cli-schemas.tstests/agent/agent-executor.test.tstests/agent/list-directory.test.tstests/agent/merger.test.tstests/agent/path-utils.test.tstests/agent/read-file.test.tstests/agent/search-content.test.tstests/agent/search-files.test.tstests/agent/types.test.ts
- Return repo-root-relative search_files paths for direct tool chaining. - Warn and fall back to JSON when rdjson/vale-json is selected in agent mode, avoiding empty formatter output. - Harden path containment checks, regex validation, and agent structured-output fallback parsing. - Convert list_directory to async fs/promises behavior and remove redundant test setup. - Add and align agentic design/plan docs with the updated contracts.
- Map agent findings into rdjson diagnostics for inline review tooling. - Keep vale-json unsupported in agent mode with JSON fallback warning. - Update agentic design/plan docs to reflect rdjson compatibility. - Add orchestrator tests for rdjson mapping and vale-json fallback.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
docs/plans/2026-03-17-agentic-capabilities.md (1)
276-282: Plan'sisWithinRootdiffers from actual implementation.The code snippet in this plan shows a simple
path.resolveapproach, but the actual implementation insrc/agent/tools/path-utils.tsusesrealpathSyncto resolve symlinks before checking containment. This is an improvement over the plan—consider updating the plan to reflect the actual more secure implementation for documentation accuracy.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/plans/2026-03-17-agentic-capabilities.md` around lines 276 - 282, Update the plan's isWithinRoot example to match the real implementation: note that the actual function uses realpathSync to resolve symlinks before containment checks (see function isWithinRoot in path-utils.ts) rather than just path.resolve; change the snippet and description to mention realpathSync-based resolution and the security rationale so docs reflect the actual, safer behavior.src/agent/agent-executor.ts (1)
184-193: Fallback parsing uses potentially emptyresponseTexton early errors.When
generateTextthrows before completion,responseTextremains an empty string (from line 117). TheNoOutputGeneratedErrorfallback at line 186 will then callparseAgentOutputFromText(''), which returnsnulland falls through to the generic error. This is safe but could be optimized by checkingresponseTextfirst.🔧 Minor optimization
} catch (error) { if (NoOutputGeneratedError.isInstance(error)) { + if (responseText) { const fallbackOutput = parseAgentOutputFromText(responseText); if (fallbackOutput) { return { findings: fallbackOutput.findings, ruleId: rule.meta.id, }; } + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/agent/agent-executor.ts` around lines 184 - 193, The catch block handling NoOutputGeneratedError should avoid calling parseAgentOutputFromText when responseText is empty; update the catch (error) logic around NoOutputGeneratedError.isInstance(error) to first check that responseText (the variable populated during generateText) is non-empty/has non-whitespace content (e.g., responseText && responseText.trim()) before calling parseAgentOutputFromText(responseText), and only return the fallback { findings: ..., ruleId: rule.meta.id } when parseAgentOutputFromText actually returns a value; otherwise fall through to the existing generic error path.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/specs/2026-03-17-agentic-capabilities-design.md`:
- Around line 150-179: The executor is not threading the orchestrator
AbortSignal into tool calls—generateText already receives signal but the tool
wrappers call tool.execute(params) without a signal; update the Tool
interface/method signature so execute(id, params, signal?: AbortSignal) accepts
a signal, and change all invocation sites in agent-executor.ts (the wrapper
functions that call tool.execute) to forward the orchestrator signal (the same
controller.signal used for generateText) into tool.execute; also update any tool
implementations to honor the signal (entry check, event listener, and manual
checkpoints) so long-running tool operations can be aborted.
In `@src/agent/tools/search-content.ts`:
- Around line 177-182: The code mixes rejection and resolution for errors:
isWithinRoot check uses Promise.reject(new Error(...)) while isLikelyUnsafeRegex
returns Promise.resolve('Invalid regex pattern'), causing inconsistent caller
behavior; change the path traversal branch so it also resolves with a
user-facing string (e.g., return Promise.resolve('Path traversal blocked:
${searchDir} is outside the allowed root')) instead of rejecting, keeping error
reporting consistent for callers of search-content.ts and using the same
message-style as the isLikelyUnsafeRegex branch; update references in tests or
callers that expect a rejection from isWithinRoot if any.
- Around line 110-114: The glob sync call in searchWithJs returns files that can
follow symlinks, allowing traversal outside the intended searchRoot; update the
fg.sync options used in searchWithJs to include followSymbolicLinks: false
(matching search-files.ts) so that symbolic links are not followed and external
targets cannot be read despite isWithinRoot checks on the root; ensure the
option is added to the same options object that sets cwd, ignore, and absolute.
In `@src/cli/orchestrator.ts`:
- Around line 145-154: The call to runWithConcurrency uses Math.max(1,
concurrency) but concurrency can be undefined which makes Math.max return NaN;
replace the expression by computing a safe integer worker count before the call
(e.g., coerce options.concurrency to a number with a default and ensure it's at
least 1) and then pass that safe value into runWithConcurrency; update the code
around the runWithConcurrency invocation that references concurrency (and any
options.concurrency source) so you use the validated safeConcurrency variable
instead of Math.max(1, concurrency).
---
Nitpick comments:
In `@docs/plans/2026-03-17-agentic-capabilities.md`:
- Around line 276-282: Update the plan's isWithinRoot example to match the real
implementation: note that the actual function uses realpathSync to resolve
symlinks before containment checks (see function isWithinRoot in path-utils.ts)
rather than just path.resolve; change the snippet and description to mention
realpathSync-based resolution and the security rationale so docs reflect the
actual, safer behavior.
In `@src/agent/agent-executor.ts`:
- Around line 184-193: The catch block handling NoOutputGeneratedError should
avoid calling parseAgentOutputFromText when responseText is empty; update the
catch (error) logic around NoOutputGeneratedError.isInstance(error) to first
check that responseText (the variable populated during generateText) is
non-empty/has non-whitespace content (e.g., responseText && responseText.trim())
before calling parseAgentOutputFromText(responseText), and only return the
fallback { findings: ..., ruleId: rule.meta.id } when parseAgentOutputFromText
actually returns a value; otherwise fall through to the existing generic error
path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 936058df-12e0-42a1-950e-d24be12f6448
📒 Files selected for processing (13)
docs/plans/2026-03-17-agentic-capabilities.mddocs/specs/2026-03-17-agentic-capabilities-design.mdsrc/agent/agent-executor.tssrc/agent/tools/list-directory.tssrc/agent/tools/path-utils.tssrc/agent/tools/search-content.tssrc/agent/tools/search-files.tssrc/cli/orchestrator.tssrc/output/reporter.tstests/agent/agent-executor.test.tstests/agent/list-directory.test.tstests/agent/search-content.test.tstests/agent/search-files.test.ts
✅ Files skipped from review due to trivial changes (3)
- tests/agent/agent-executor.test.ts
- tests/agent/search-files.test.ts
- src/agent/tools/path-utils.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- src/output/reporter.ts
- tests/agent/search-content.test.ts
- tests/agent/list-directory.test.ts
- src/agent/tools/search-files.ts
- src/agent/tools/list-directory.ts
- disable symlink following in JS content search fallback to keep traversal constraints consistent - sanitize agent-mode concurrency input to a safe positive integer before scheduling work - align design spec wording with current v1 cancellation behavior (model-level signal only)
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/cli/orchestrator.ts (1)
203-212:⚠️ Potential issue | 🟡 Minor
concurrencymay beNaNif undefined, causing zero workers.At line 203,
Math.max(1, concurrency)is used, but ifoptions.concurrencyisundefined,Math.max(1, undefined)returnsNaN. This causesrunWithConcurrencyto create zero workers sinceMath.min(NaN, items.length)evaluates toNaN, which becomes 0 in array creation.🔧 Proposed fix with nullish coalescing
- const agentResults = await runWithConcurrency(prompts, Math.max(1, concurrency), async (rule) => + const agentResults = await runWithConcurrency(prompts, Math.max(1, concurrency ?? 1), async (rule) =>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cli/orchestrator.ts` around lines 203 - 212, The Math.max(1, concurrency) call can produce NaN when concurrency is undefined; update the orchestration call to coerce undefined/null to a numeric default before passing to runWithConcurrency (e.g., use a nullish coalescing fallback for options.concurrency) so the worker count is always at least 1. Locate the call around runWithConcurrency(prompts, Math.max(1, concurrency), ...) in orchestrator.ts and replace Math.max(1, concurrency) with Math.max(1, (concurrency ?? 1)) or an equivalent numeric fallback; ensure you reference the same concurrency variable derived from options.concurrency and that runAgentExecutor and prompts usage remains unchanged. Ensure the final value passed is a finite integer >= 1 so runWithConcurrency's internal Math.min/array creation cannot produce NaN.
🧹 Nitpick comments (1)
docs/specs/2026-03-17-agentic-capabilities-design.md (1)
45-60: Consider adding a language specifier to the fenced code block.The architecture diagram uses a plain fenced code block without a language specifier. While this is a minor formatting issue, adding a specifier like
textorplaintextwould satisfy linters.-``` +```text GitHub App │🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/specs/2026-03-17-agentic-capabilities-design.md` around lines 45 - 60, The fenced code block containing the architecture diagram (lines showing "GitHub App", "VectorLint Cloud", "VectorLint Service", "VectorLint CLI", "Lint Executor", and "Agent Executor") should include a language specifier to satisfy linters—update the opening fence from ``` to ```text (or ```plaintext) so the diagram is treated as plain text; ensure the rest of the block content is unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/cli/orchestrator.ts`:
- Around line 203-212: The Math.max(1, concurrency) call can produce NaN when
concurrency is undefined; update the orchestration call to coerce undefined/null
to a numeric default before passing to runWithConcurrency (e.g., use a nullish
coalescing fallback for options.concurrency) so the worker count is always at
least 1. Locate the call around runWithConcurrency(prompts, Math.max(1,
concurrency), ...) in orchestrator.ts and replace Math.max(1, concurrency) with
Math.max(1, (concurrency ?? 1)) or an equivalent numeric fallback; ensure you
reference the same concurrency variable derived from options.concurrency and
that runAgentExecutor and prompts usage remains unchanged. Ensure the final
value passed is a finite integer >= 1 so runWithConcurrency's internal
Math.min/array creation cannot produce NaN.
---
Nitpick comments:
In `@docs/specs/2026-03-17-agentic-capabilities-design.md`:
- Around line 45-60: The fenced code block containing the architecture diagram
(lines showing "GitHub App", "VectorLint Cloud", "VectorLint Service",
"VectorLint CLI", "Lint Executor", and "Agent Executor") should include a
language specifier to satisfy linters—update the opening fence from ``` to
```text (or ```plaintext) so the diagram is treated as plain text; ensure the
rest of the block content is unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: cadb2400-b535-4345-9185-ccd8371ae07e
📒 Files selected for processing (4)
docs/plans/2026-03-17-agentic-capabilities.mddocs/specs/2026-03-17-agentic-capabilities-design.mdsrc/cli/orchestrator.tstests/orchestrator-agent-output.test.ts
- stop stat'ing every directory entry once the requested limit is satisfied - compute truncation hint from raw entry count to avoid false positives - normalize diff-context file rendering through repo-relative path helper - add tests for truncation hint behavior
- run each agent with rule-specific matched files derived from scan path resolution - switch lint tool contract to file+ruleContent+context and bind lint tool per rule - classify top-level findings as warnings (including rdjson severity and summary counts) - add tests covering matched-file routing and warning/error aggregation
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/agent/tools/search-content.ts (1)
178-183:⚠️ Potential issue | 🟡 MinorInconsistent error handling: path traversal rejects while unsafe regex resolves.
Path traversal at line 179 uses
Promise.reject(new Error(...))while unsafe regex at line 182 usesPromise.resolve('Invalid regex pattern'). For agent tools that return strings for LLM consumption, both should resolve with error messages for consistent caller behavior.🔧 Proposed fix
if (!isWithinRoot(searchRoot, cwd)) { - return Promise.reject(new Error(`Path traversal blocked: ${searchDir} is outside the allowed root`)); + return Promise.resolve(`Path traversal blocked: ${searchDir} is outside the allowed root`); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/agent/tools/search-content.ts` around lines 178 - 183, The code mixes rejection and resolution for input validation: isWithinRoot(...) currently does Promise.reject(new Error(...)) while isLikelyUnsafeRegex(...) returns Promise.resolve('Invalid regex pattern'); change the path-traversal branch to return a resolved string error like 'Path traversal blocked: <searchDir> is outside the allowed root' (i.e., replace Promise.reject with Promise.resolve returning a plain error string) so both isWithinRoot and isLikelyUnsafeRegex validation paths consistently resolve with human-readable error strings for the caller; update the logic around searchRoot, cwd, searchDir, pattern, isWithinRoot, and isLikelyUnsafeRegex accordingly.
🧹 Nitpick comments (4)
src/cli/orchestrator.ts (2)
271-310: Consider adding AbortSignal to agent executor calls for graceful cancellation.The
runAgentModefunction doesn't create or pass anAbortSignaltorunAgentExecutor, even though the interface supports it (line 30 in agent-executor.ts). Without this, there's no way to implement wall-clock timeouts for runaway agent loops as mentioned in the design spec (line 120).For v1 this may be acceptable per the spec, but consider adding a timeout mechanism:
const controller = new AbortController(); const timeout = setTimeout(() => controller.abort(), AGENT_TIMEOUT_MS); try { // ... runAgentExecutor calls with signal: controller.signal } finally { clearTimeout(timeout); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cli/orchestrator.ts` around lines 271 - 310, Add an AbortSignal to agent runs so callers can cancel long-running agents: in runAgentMode create an AbortController (e.g., controller) before invoking runWithConcurrency, start a timeout that calls controller.abort() after your configured AGENT_TIMEOUT_MS, pass controller.signal through to each runAgentExecutor call in the tools/args object, and ensure you clear the timeout in a finally block; update usages around runWithConcurrency, runAgentExecutor, and agentResults to thread the signal through.
1-3: Consider usingnode:protocol for Node.js built-in imports.The agent-related files (
src/agent/tools/search-content.ts, etc.) usenode:fs,node:path,node:child_process. For consistency across the codebase, consider updating these imports to use the explicitnode:protocol.📝 Proposed fix
-import { readFileSync } from 'fs'; -import { randomUUID } from 'crypto'; -import * as path from 'path'; +import { readFileSync } from 'node:fs'; +import { randomUUID } from 'node:crypto'; +import * as path from 'node:path';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cli/orchestrator.ts` around lines 1 - 3, The import statements in orchestrator.ts use bare specifiers for built-ins; update them to the explicit node: protocol to match the rest of the codebase (change "fs" -> "node:fs", "crypto" -> "node:crypto", and "path" -> "node:path") so that the existing imports like readFileSync, randomUUID and the path namespace import remain identical but use the node: protocol for consistency with files such as src/agent/tools/search-content.ts.src/agent/agent-executor.ts (2)
119-121: Redundant default value formatchedFilesin destructuring.
matchedFilesis already typed asstring[]inAgentExecutorParams(not optional), so the= []default is never reached. This could mask caller bugs if the type changes to optional later.Consider either:
- Remove the default if the field is always required
- Make the field optional in the interface if defaults are intended
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/agent/agent-executor.ts` around lines 119 - 121, The destructured default value "matchedFiles = []" in runAgentExecutor is redundant because AgentExecutorParams currently defines matchedFiles as a non-optional string[]; remove the "= []" default from the destructuring in runAgentExecutor so the function relies on the declared type, or alternatively update AgentExecutorParams to make matchedFiles optional (matchedFiles?: string[]) if you intend a default to be applied — ensure you update any callers/tests accordingly and keep the symbol names runAgentExecutor and AgentExecutorParams consistent.
190-207: Add explicitunknowntype to catch block parameter.Per coding guidelines, catch blocks should use
unknowntype. While the current code handles errors correctly, the explicit type annotation improves clarity and enforces the guideline.📝 Proposed fix
- } catch (error) { + } catch (error: unknown) { if (NoOutputGeneratedError.isInstance(error)) {As per coding guidelines: "catch blocks use
unknowntype".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/agent/agent-executor.ts` around lines 190 - 207, Change the catch parameter to use the explicit unknown type: replace catch (error) with catch (error: unknown) in the try/catch block inside agent-executor (the block that references NoOutputGeneratedError, parseAgentOutputFromText, responseText and rule.meta.id); keep the existing runtime checks (NoOutputGeneratedError.isInstance(error) and error instanceof Error) and the fallback handling so the code still narrows the unknown to Error or string before constructing the message and returning the error payload.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/specs/2026-03-17-agentic-capabilities-design.md`:
- Around line 45-60: The fenced ASCII diagram block in the docs (the
triple-backtick block showing "GitHub App → VectorLint Cloud → VectorLint
Service → VectorLint CLI...") lacks a language specifier and triggers MD040;
update that code fence to include a text language identifier (```text) so the
diagram is treated as plain text/ASCII art—locate the triple-backtick block
containing the diagram and change the opening fence to use "text".
---
Duplicate comments:
In `@src/agent/tools/search-content.ts`:
- Around line 178-183: The code mixes rejection and resolution for input
validation: isWithinRoot(...) currently does Promise.reject(new Error(...))
while isLikelyUnsafeRegex(...) returns Promise.resolve('Invalid regex pattern');
change the path-traversal branch to return a resolved string error like 'Path
traversal blocked: <searchDir> is outside the allowed root' (i.e., replace
Promise.reject with Promise.resolve returning a plain error string) so both
isWithinRoot and isLikelyUnsafeRegex validation paths consistently resolve with
human-readable error strings for the caller; update the logic around searchRoot,
cwd, searchDir, pattern, isWithinRoot, and isLikelyUnsafeRegex accordingly.
---
Nitpick comments:
In `@src/agent/agent-executor.ts`:
- Around line 119-121: The destructured default value "matchedFiles = []" in
runAgentExecutor is redundant because AgentExecutorParams currently defines
matchedFiles as a non-optional string[]; remove the "= []" default from the
destructuring in runAgentExecutor so the function relies on the declared type,
or alternatively update AgentExecutorParams to make matchedFiles optional
(matchedFiles?: string[]) if you intend a default to be applied — ensure you
update any callers/tests accordingly and keep the symbol names runAgentExecutor
and AgentExecutorParams consistent.
- Around line 190-207: Change the catch parameter to use the explicit unknown
type: replace catch (error) with catch (error: unknown) in the try/catch block
inside agent-executor (the block that references NoOutputGeneratedError,
parseAgentOutputFromText, responseText and rule.meta.id); keep the existing
runtime checks (NoOutputGeneratedError.isInstance(error) and error instanceof
Error) and the fallback handling so the code still narrows the unknown to Error
or string before constructing the message and returning the error payload.
In `@src/cli/orchestrator.ts`:
- Around line 271-310: Add an AbortSignal to agent runs so callers can cancel
long-running agents: in runAgentMode create an AbortController (e.g.,
controller) before invoking runWithConcurrency, start a timeout that calls
controller.abort() after your configured AGENT_TIMEOUT_MS, pass
controller.signal through to each runAgentExecutor call in the tools/args
object, and ensure you clear the timeout in a finally block; update usages
around runWithConcurrency, runAgentExecutor, and agentResults to thread the
signal through.
- Around line 1-3: The import statements in orchestrator.ts use bare specifiers
for built-ins; update them to the explicit node: protocol to match the rest of
the codebase (change "fs" -> "node:fs", "crypto" -> "node:crypto", and "path" ->
"node:path") so that the existing imports like readFileSync, randomUUID and the
path namespace import remain identical but use the node: protocol for
consistency with files such as src/agent/tools/search-content.ts.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5d85a6e0-16b6-492d-8ec1-d6af19bbfbe1
📒 Files selected for processing (10)
docs/specs/2026-03-17-agentic-capabilities-design.mdsrc/agent/agent-executor.tssrc/agent/tools/lint-tool.tssrc/agent/tools/list-directory.tssrc/agent/tools/search-content.tssrc/cli/orchestrator.tssrc/output/reporter.tstests/agent/agent-executor.test.tstests/agent/list-directory.test.tstests/orchestrator-agent-output.test.ts
✅ Files skipped from review due to trivial changes (3)
- src/agent/tools/list-directory.ts
- tests/agent/agent-executor.test.ts
- tests/orchestrator-agent-output.test.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- tests/agent/list-directory.test.ts
- src/output/reporter.ts
- src/agent/tools/lint-tool.ts
Comment on lines
+45
to
+60
| ``` | ||
| GitHub App | ||
| │ | ||
| ▼ | ||
| VectorLint Cloud ← receives webhook, sends credentials + PR metadata | ||
| │ | ||
| ▼ | ||
| VectorLint Service ← shallow clones repo, invokes CLI | ||
| │ | ||
| ▼ | ||
| VectorLint CLI | ||
| ├── Lint Executor ← default mode: existing per-page evaluation (unchanged) | ||
| └── Agent Executor ← --mode agent: one agent per rule, parallel | ||
| │ | ||
| └── Tools: lint, read_file, search_content, search_files, list_directory | ||
| ``` |
There was a problem hiding this comment.
Add language specifier to fenced code block.
The architecture diagram code block lacks a language specifier. Static analysis flagged this (MD040). Use a text-based identifier for ASCII diagrams.
📝 Proposed fix
-```
+```text
GitHub App
│
▼📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| ``` | |
| GitHub App | |
| │ | |
| ▼ | |
| VectorLint Cloud ← receives webhook, sends credentials + PR metadata | |
| │ | |
| ▼ | |
| VectorLint Service ← shallow clones repo, invokes CLI | |
| │ | |
| ▼ | |
| VectorLint CLI | |
| ├── Lint Executor ← default mode: existing per-page evaluation (unchanged) | |
| └── Agent Executor ← --mode agent: one agent per rule, parallel | |
| │ | |
| └── Tools: lint, read_file, search_content, search_files, list_directory | |
| ``` |
🧰 Tools
🪛 markdownlint-cli2 (0.21.0)
[warning] 45-45: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/specs/2026-03-17-agentic-capabilities-design.md` around lines 45 - 60,
The fenced ASCII diagram block in the docs (the triple-backtick block showing
"GitHub App → VectorLint Cloud → VectorLint Service → VectorLint CLI...") lacks
a language specifier and triggers MD040; update that code fence to include a
text language identifier (```text) so the diagram is treated as plain text/ASCII
art—locate the triple-backtick block containing the diagram and change the
opening fence to use "text".
- add lint-tool contract tests for ruleContent validation and context injection - add fixture-backed agent trace replay test through real orchestrator/executor flow - expand scan-path routing tests for rule disable overrides across overlapping patterns
- compute post-loop agent rule scores from inline findings using density scoring - exclude top-level findings from scoring while keeping them as warning-level findings - make search_files honor repo .gitignore patterns and keep repo-relative output - add tests for scoring behavior and gitignore-aware file discovery
- drop plan, spec, and execution log docs from this PR scope - keep implementation changes focused on agent runtime and tests
- switch line output progress to a single static spinner message - render one final done line and keep status output on stderr only - keep json, rdjson, and vale-json stdout payload behavior unchanged - add focused reporter and orchestrator tests for tty and stdout safety
- show linting text in lint mode and reviewing text in agent mode - stop spinner before report rows so output starts on a fresh line - add tests covering mode-specific status text and newline behavior
- run agent-mode rules serially and limit tool execution to one at a time - add tool-level status updates during agent runs for better visibility - make agent retries configurable via AGENT_MAX_RETRIES in config.toml - add and update tests for retry wiring, env parsing, and status output
Content Review by VectorLintNo issues found!Great job! Your content passed all VectorLint checks. Powered by VectorLint |
- Reuse lint row rendering for agent findings to keep line output consistent. - Map top-level agent findings to 1:1 and group rows by file headers. - Replace raw status tokens with human-readable agent tool actions. - Show elapsed time in the final spinner completion line. - Update progress and agent output tests for the new UX contract.
- Centralize agent tool-action labels in a single constant map. - Eliminate intermediate finding normalization array in row rendering. - Skip spinner re-render when running text is unchanged. - Preserve output behavior while reducing redundant work.
- Switch agent execution ordering to file-first units (file + rule). - Replace single-line spinner with TTY block progress per file, with live tool-call updates that preserve scrollback history. - Surface tool args in status events and format compact tool lines (including concise lint preview display). - Expand agent output tests for file-first orchestration and block-progress rendering behavior.
- replace per-file-per-rule executor model with one agent session per run - define strict prompt precedence: role, operating policy, runtime context - add global -p/--print headless mode contract for lint and agent - clarify naming semantics to avoid git-diff terminology drift
- replace broad docs ignore with explicit docs/logs, docs/artifacts, docs/plans paths - keep docs/specs trackable for versioned design updates
- replace per-file/per-rule executor args with requestedTargets + fileRuleMap - add global -p/--print plumbing and suppress interactive progress in print mode - support run-level lint routing by ruleId via rules catalog - update agent and orchestrator tests for single-run behavior and prompt precedence
- route lint by catalog ruleKey to avoid ruleId collision ambiguity - remove synthetic post-run file/rule progress transitions - add parser coverage for print flag and expand lint-tool routing tests
- Increase agent-mode default max retries from 5 to 10 for better resilience under transient provider and rate-limit failures. - Keep AGENT_MAX_RETRIES configurable and update the global config template to reflect the new default. - Update executor tests to assert the new retry default.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
VectorLint needed a cross-document evaluation path that can reason across files while still producing review-friendly output (including rdjson diagnostics). Existing per-file lint mode alone could not reliably surface structural/off-page documentation issues during PR review.
What
--mode agentorchestration in the CLI, running one agent execution per active rule.read_file,search_content,search_files,list_directory, andlint.inlineandtop-level) plus merger and reporter support.line,json,rdjson, andvale-jsonfallback-to-json behavior.file + ruleContent + optional context, with rule-scoped lint tool instances.Scope
In scope
Out of scope
judge/technical-accuracypathways in non-agent lint flows.query_codebasetool.Behavior impact
--mode agentis used.rdjsonoutput now carries agent findings with appropriate severity for inline comment tooling.Risk
How to test / verify
Checks run
npm run lintnpm run test:runnpm run test:run -- tests/agent/agent-executor.test.ts tests/orchestrator-agent-output.test.tsManual verification
npm run dev -- --mode agent --output line <target-paths>and confirm both inline and top-level findings render.npm run dev -- --mode agent --output rdjson <target-paths>and confirm diagnostics include severity and paths suitable for inline annotations.npm run dev -- --mode agent --output vale-json <target-paths>and confirm fallback warning + JSON payload.Rollback
--mode agentto continue using existing lint-only behavior.