feat(agent): redesign agent mode with deterministic replay runtime#76
Closed
oshorefueled wants to merge 9 commits intomainfrom
Closed
feat(agent): redesign agent mode with deterministic replay runtime#76oshorefueled wants to merge 9 commits intomainfrom
oshorefueled wants to merge 9 commits intomainfrom
Conversation
Contributor
Author
|
@coderabbitai review |
📝 WalkthroughWalkthroughThis change introduces a complete agent-mode runtime for conducting code reviews via tooling. It wires CLI options ( Changes
Sequence DiagramsequenceDiagram
participant User as User
participant CLI as CLI Orchestrator
participant Executor as Agent Executor
participant Store as Session Store
participant Tool as Lint Tool
participant Model as Agent Model
User->>CLI: evaluateFiles(targets, {mode: 'agent'})
CLI->>Executor: runAgentExecutor({targets, prompts})
Executor->>Store: createReviewSessionStore()
Store-->>Executor: ReviewSessionStore
Executor->>Store: append(session_started)
Store-->>Executor: ✓
Executor->>Executor: buildRuleRegistry(targets, prompts)
Executor->>Executor: createLintTool(registry)
Executor->>Model: executeAgent({lint, report_finding, finalize_review})
rect rgba(100, 200, 255, 0.5)
Note over Model,Tool: Tool Execution Loop
Model->>Tool: lint({file, ruleSource})
Tool->>Tool: registry.lookup(ruleSource)
Tool-->>Model: LintToolResult
Executor->>Store: append(finding_recorded_inline)
Store-->>Executor: ✓
end
rect rgba(100, 255, 200, 0.5)
Note over Model,Executor: Finalization
Model->>Executor: finalize_review({totalFindings})
Executor->>Store: append(session_finalized)
Store-->>Executor: ✓
end
Executor->>Store: replay()
Store-->>Executor: SessionEvent[]
Executor->>Executor: buildAgentReplayReport()
Executor-->>CLI: RunAgentExecutorResult
CLI->>CLI: print report (if JSON format)
CLI-->>User: EvaluationResult
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
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 docstrings
🧪 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 |
✅ Actions performedReview triggered.
|
![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
🧹 Nitpick comments (10)
src/agent/session-replay.ts (2)
18-22: Duplicate implementation ofreplaySessionEvents.This function is nearly identical to the
replay()method insrc/agent/review-session-store.ts(lines 57-61). Consider either:
- Exporting
replayfrom the store and reusing it here, or- Extracting a shared parsing utility.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/agent/session-replay.ts` around lines 18 - 22, The function replaySessionEvents duplicates logic already present in ReviewSessionStore.replay; to fix, either export and reuse the existing replay method from src/agent/review-session-store.ts (call ReviewSessionStore.replay or export a standalone replay function) or extract the JSON-lines-to-SessionEvent parsing into a shared utility (e.g., parseSessionEventsFromFile) and call that from both replaySessionEvents and ReviewSessionStore.replay; update imports and remove the duplicate implementation so parsing lives in one place and both callers delegate to it.
28-35: Unnecessary O(n²) complexity from per-event merge.Calling
mergeAgentFindings(findings, [event.payload.finding])for each event recreates the fingerprintSetfrom scratch on every iteration. Since events in a single session file are already recorded in order and should not contain duplicates (the executor de-duplicates before appending), you could simplify to a direct push, or build the Set once outside the loop.♻️ Simplified accumulation
export async function replaySessionFindings(sessionFilePath: string): Promise<AgentFinding[]> { const events = await replaySessionEvents(sessionFilePath); - let findings: AgentFinding[] = []; + const findings: AgentFinding[] = []; + const seen = new Set<string>(); for (const event of events) { if ( event.eventType === "finding_recorded_inline" || event.eventType === "finding_recorded_top_level" ) { - findings = mergeAgentFindings(findings, [event.payload.finding]); + const finding = event.payload.finding; + const key = JSON.stringify([finding.kind, finding.ruleId, finding.ruleSource, finding.file ?? "", finding.line ?? null, finding.message]); + if (!seen.has(key)) { + seen.add(key); + findings.push(finding); + } } } return findings; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/agent/session-replay.ts` around lines 28 - 35, The loop is causing O(n²) work by calling mergeAgentFindings(findings, [event.payload.finding]) for every event; instead, accumulate findings directly (e.g., push each event.payload.finding into the findings array or collect into a local array or Set) and call mergeAgentFindings once (or build the fingerprint Set once) after the loop; update the loop over events to reference event.payload.finding and stop calling mergeAgentFindings per-iteration to avoid reconstructing the fingerprint Set repeatedly.src/agent/review-session-store.ts (2)
82-85:hasFinalizedEvent()re-reads entire file on each call.This method replays all events just to check if any is
session_finalized. For long sessions with many events, this becomes increasingly expensive. If this is called frequently (e.g., in a polling loop), consider caching the finalized state after the firsttrueresult, since finalization is a terminal state.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/agent/review-session-store.ts` around lines 82 - 85, hasFinalizedEvent currently calls replay() and scans all events each time; change it to memoize the terminal finalized state so repeated calls don't re-read the whole event stream. Add a private boolean (e.g., _isFinalized) initialized false on the store, update it to true whenever replay() finds event.eventType === 'session_finalized' or when any code path that emits finalization runs, and have hasFinalizedEvent return the cached _isFinalized immediately if true; only call replay() if _isFinalized is false and then set the cache when a finalized event is observed. Ensure you reference hasFinalizedEvent and replay in the change and keep cache updates atomic/consistent with any write/finalize methods.
53-56: Minor TOCTOU window between file creation and first append.Opening with
'wx'then immediately closing creates a brief window where another process could theoretically write to the file before the firstappend(). In practice, the uniquesessionIdmakes this extremely unlikely. If stronger guarantees are needed, consider keeping the file handle open for the session lifetime.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/agent/review-session-store.ts` around lines 53 - 56, The current pattern opens the file with open(sessionFilePath, 'wx') then immediately calls handle.close(), creating a TOCTOU window before the first append; instead, keep the file handle open for the session lifetime and use it for subsequent writes to avoid the race. Update the code around open(sessionFilePath, 'wx')/handle.close() in review-session-store.ts to store the returned handle (e.g., in a map keyed by sessionId or as part of the session object), perform writes against that handle (using handle.write or handle.appendFile) for subsequent append() operations, and ensure the handle is closed when the session ends or is cleaned up. Ensure append() uses the kept handle rather than re-opening the file so there is no window between creation and first write.src/agent/merger.ts (1)
3-11: Consider using a delimiter-free serialization for fingerprint.The
::delimiter infindingFingerprintcould cause false-positive collisions if any field (particularlymessage) contains the literal string::. For example, two different findings could produce the same fingerprint if one field's value contains::that aligns with adjacent field boundaries.A safer approach would use JSON serialization or a delimiter that's escaped/unlikely in content.
♻️ Optional: Use JSON for collision-safe fingerprinting
function findingFingerprint(finding: AgentFinding): string { - return [ - finding.kind, - finding.ruleId, - finding.ruleSource, - finding.file ?? "", - String(finding.line ?? ""), - finding.message, - ].join("::"); + return JSON.stringify([ + finding.kind, + finding.ruleId, + finding.ruleSource, + finding.file ?? "", + finding.line ?? null, + finding.message, + ]); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/agent/merger.ts` around lines 3 - 11, The current findingFingerprint function uses "::" delimiter which can collide when field values contain that sequence; change findingFingerprint to produce a collision-safe serialization (e.g., use JSON.stringify on an object with stable keys or encode/escape each field) so the fingerprint is unambiguous; update the function findingFingerprint to serialize { kind, ruleId, ruleSource, file, line, message } with JSON.stringify (or an escape scheme) to ensure deterministic ordering and avoid delimiter collisions while keeping the same return type (string).src/agent/types.ts (1)
102-109: DuplicatetoPascalCaseimplementation.This
toPascalCasefunction is identical to the one insrc/prompts/rule-identity.ts(lines 4-11). Consider extracting it to a shared utility module to avoid duplication and ensure consistent behavior across the codebase.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/agent/types.ts` around lines 102 - 109, The toPascalCase function is duplicated; extract it into a single shared utility (e.g., export a function named toPascalCase from a new/shared utils module), replace the local implementations in the agent types and the rule-identity usage with an import of that exported toPascalCase, remove the duplicate definitions, and run/typecheck to ensure no signature or behavior changes; reference the existing symbol name toPascalCase and update any imports that previously referenced the local implementations.tests/orchestrator-agent-output.test.ts (1)
232-237: Consider extracting progress format constants to reduce test brittleness.The exact substring assertions (e.g.,
'⠋ ◈ reviewing','└ calling tool lint tool lint') are tightly coupled to the progress reporter's output format. If the progress messages change, these tests will fail even though the functionality is correct.Consider either:
- Extracting progress format strings to shared constants used by both the reporter and tests
- Using regex patterns that match the semantic structure rather than exact formatting
This is a minor maintainability concern and can be addressed later.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/orchestrator-agent-output.test.ts` around lines 232 - 237, Tests assert exact progress strings from stderrOutput (e.g., the REVIEW and LINT messages using README_PATH), which is brittle; change the test to either reference shared progress format constants defined in the progress reporter module or relax assertions to semantic regexes: replace the exact substring checks on stderrOutput with patterns that match the semantic pieces (e.g., presence of "reviewing", the README_PATH, "calling tool lint", and "done" timestamps) or import constants exported by the progress reporter (progress symbols/format strings) and use those constants in the assertions so the test uses the canonical format instead of hardcoded literals.src/agent/agent-executor.ts (1)
84-91: Redundant initialization check forfileRuleMapentries.Lines 65-67 already initialize
fileRuleMap[file] = []for all targets, so the existence check at lines 85-89 will never create new entries. The code works correctly but the conditional is dead logic.♻️ Simplified version
for (const file of targets) { - let rulesForFile = fileRuleMap[file]; - if (!rulesForFile) { - rulesForFile = []; - fileRuleMap[file] = rulesForFile; - } - rulesForFile.push(ruleSource); + fileRuleMap[file].push(ruleSource); }🤖 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 84 - 91, The loop in agent-executor.ts redundantly checks/initializes entries in fileRuleMap even though earlier code already set fileRuleMap[file] = [] for all targets; remove the dead conditional in the loop and directly push ruleSource into the existing array. Locate the for (const file of targets) loop that references fileRuleMap, rulesForFile and ruleSource, and replace the conditional block that creates rulesForFile with a straight rulesForFile = fileRuleMap[file]; rulesForFile.push(ruleSource) usage (or directly fileRuleMap[file].push(ruleSource)) to simplify and eliminate dead logic.src/agent/index.ts (1)
40-45: Unsafe type assertions bypass type checking.The
as RunAgentExecutorParams['runRule']andas RunAgentExecutorParams['executeAgent']casts silence type mismatches betweenAgentExecutionOptions(which usesunknownparameters insrc/cli/types.ts) andRunAgentExecutorParams(which expects specific typed parameters). If the signatures diverge, these casts will mask runtime errors.Consider either:
- Aligning the types in
AgentExecutionOptionswithRunAgentExecutorParams- Adding runtime validation before the cast
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/agent/index.ts` around lines 40 - 45, The current unsafe casts on options.agent?.runRule and options.agent?.execute bypass TypeScript checks (using as RunAgentExecutorParams['runRule'] / ['executeAgent']); either align AgentExecutionOptions to use the exact RunAgentExecutorParams types or add runtime validation before converting: implement validation helpers (e.g., validateRunRuleParams and validateExecuteAgentParams) that assert the shape/types expected by RunAgentExecutorParams, call them where options.agent?.runRule and options.agent?.execute are read, and only include the properties in the spread if validation passes (otherwise log/throw); alternatively, update the AgentExecutionOptions definitions to use the exact RunAgentExecutorParams types so the casts are unnecessary.docs/logs/2026-03-31-agent-mode-implementation-plan.log.md (1)
19-19: Minor grammar: hyphenate compound adjective."rule-source based" should be "rule-source-based" when used as a compound adjective before "tool schemas".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/logs/2026-03-31-agent-mode-implementation-plan.log.md` at line 19, Update the compound adjective "rule-source based" to the hyphenated form "rule-source-based" in the sentence that reads "Introduced rule-source based tool schemas, canonical `Pack.Rule` normalization, and a typed session event union including explicit finalization events for deterministic replay contracts."; replace "rule-source based" with "rule-source-based" to correct the grammar while leaving the rest of the sentence and the `Pack.Rule` reference 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/index.ts`:
- Around line 49-61: The returned report currently hardcodes hadSeverityErrors
to false, preventing agent mode from exiting with a non-zero code when replay
findings have error severity; update the value of hadSeverityErrors in the
object returned by the function to derive from the replayReport (for example set
hadSeverityErrors to a boolean based on replayReport.summary.errors > 0 or the
appropriate severity count) so it reflects actual error-severity findings from
replayReport.summary; keep existing fields (totalFiles, totalErrors, etc.)
unchanged and only replace the hardcoded false with the computed boolean.
In `@src/agent/types.ts`:
- Around line 111-117: Extract the duplicated toPascalCase implementation into a
single shared utility and update callers: create a new exported function (e.g.,
toPascalCase) in a common module and import it into the files that currently
define it, then remove the duplicate implementations from both the file that
defines canonicalRuleIdFromSource and from rule-identity usage; ensure
canonicalRuleIdFromSource continues to call the shared toPascalCase and
run/update its tests, and while here evaluate whether canonicalRuleIdFromSource
is referenced anywhere in production code and, if not needed, consider
consolidating callers to use canonicalRuleIdFromPackRule instead and remove the
unused function.
In `@src/output/agent-progress-reporter.ts`:
- Around line 13-16: The compactRulePreview function always appends "..." even
when ruleSource is shorter than or equal to 48 characters; update
compactRulePreview so it only appends "..." when truncation occurs (i.e., when
ruleSource.length > 48). Locate the compactRulePreview function and change the
return logic to return the original ruleSource unchanged for short inputs, and
return the truncated slice plus "..." only when the input was actually
truncated.
- Line 53: The output string in the progress reporter duplicates the word "tool"
(see the call to options.write in agent-progress-reporter.ts); update the
message in that write call to remove the repeated word so it reads like "└
calling tool lint..." (or "└ calling lint tool..." depending on intended
phrasing) and keep the existing suffix handling intact; modify the string passed
to options.write in the function that writes the "calling tool" line to use a
single "tool" occurrence.
---
Nitpick comments:
In `@docs/logs/2026-03-31-agent-mode-implementation-plan.log.md`:
- Line 19: Update the compound adjective "rule-source based" to the hyphenated
form "rule-source-based" in the sentence that reads "Introduced rule-source
based tool schemas, canonical `Pack.Rule` normalization, and a typed session
event union including explicit finalization events for deterministic replay
contracts."; replace "rule-source based" with "rule-source-based" to correct the
grammar while leaving the rest of the sentence and the `Pack.Rule` reference
unchanged.
In `@src/agent/agent-executor.ts`:
- Around line 84-91: The loop in agent-executor.ts redundantly
checks/initializes entries in fileRuleMap even though earlier code already set
fileRuleMap[file] = [] for all targets; remove the dead conditional in the loop
and directly push ruleSource into the existing array. Locate the for (const file
of targets) loop that references fileRuleMap, rulesForFile and ruleSource, and
replace the conditional block that creates rulesForFile with a straight
rulesForFile = fileRuleMap[file]; rulesForFile.push(ruleSource) usage (or
directly fileRuleMap[file].push(ruleSource)) to simplify and eliminate dead
logic.
In `@src/agent/index.ts`:
- Around line 40-45: The current unsafe casts on options.agent?.runRule and
options.agent?.execute bypass TypeScript checks (using as
RunAgentExecutorParams['runRule'] / ['executeAgent']); either align
AgentExecutionOptions to use the exact RunAgentExecutorParams types or add
runtime validation before converting: implement validation helpers (e.g.,
validateRunRuleParams and validateExecuteAgentParams) that assert the
shape/types expected by RunAgentExecutorParams, call them where
options.agent?.runRule and options.agent?.execute are read, and only include the
properties in the spread if validation passes (otherwise log/throw);
alternatively, update the AgentExecutionOptions definitions to use the exact
RunAgentExecutorParams types so the casts are unnecessary.
In `@src/agent/merger.ts`:
- Around line 3-11: The current findingFingerprint function uses "::" delimiter
which can collide when field values contain that sequence; change
findingFingerprint to produce a collision-safe serialization (e.g., use
JSON.stringify on an object with stable keys or encode/escape each field) so the
fingerprint is unambiguous; update the function findingFingerprint to serialize
{ kind, ruleId, ruleSource, file, line, message } with JSON.stringify (or an
escape scheme) to ensure deterministic ordering and avoid delimiter collisions
while keeping the same return type (string).
In `@src/agent/review-session-store.ts`:
- Around line 82-85: hasFinalizedEvent currently calls replay() and scans all
events each time; change it to memoize the terminal finalized state so repeated
calls don't re-read the whole event stream. Add a private boolean (e.g.,
_isFinalized) initialized false on the store, update it to true whenever
replay() finds event.eventType === 'session_finalized' or when any code path
that emits finalization runs, and have hasFinalizedEvent return the cached
_isFinalized immediately if true; only call replay() if _isFinalized is false
and then set the cache when a finalized event is observed. Ensure you reference
hasFinalizedEvent and replay in the change and keep cache updates
atomic/consistent with any write/finalize methods.
- Around line 53-56: The current pattern opens the file with
open(sessionFilePath, 'wx') then immediately calls handle.close(), creating a
TOCTOU window before the first append; instead, keep the file handle open for
the session lifetime and use it for subsequent writes to avoid the race. Update
the code around open(sessionFilePath, 'wx')/handle.close() in
review-session-store.ts to store the returned handle (e.g., in a map keyed by
sessionId or as part of the session object), perform writes against that handle
(using handle.write or handle.appendFile) for subsequent append() operations,
and ensure the handle is closed when the session ends or is cleaned up. Ensure
append() uses the kept handle rather than re-opening the file so there is no
window between creation and first write.
In `@src/agent/session-replay.ts`:
- Around line 18-22: The function replaySessionEvents duplicates logic already
present in ReviewSessionStore.replay; to fix, either export and reuse the
existing replay method from src/agent/review-session-store.ts (call
ReviewSessionStore.replay or export a standalone replay function) or extract the
JSON-lines-to-SessionEvent parsing into a shared utility (e.g.,
parseSessionEventsFromFile) and call that from both replaySessionEvents and
ReviewSessionStore.replay; update imports and remove the duplicate
implementation so parsing lives in one place and both callers delegate to it.
- Around line 28-35: The loop is causing O(n²) work by calling
mergeAgentFindings(findings, [event.payload.finding]) for every event; instead,
accumulate findings directly (e.g., push each event.payload.finding into the
findings array or collect into a local array or Set) and call mergeAgentFindings
once (or build the fingerprint Set once) after the loop; update the loop over
events to reference event.payload.finding and stop calling mergeAgentFindings
per-iteration to avoid reconstructing the fingerprint Set repeatedly.
In `@src/agent/types.ts`:
- Around line 102-109: The toPascalCase function is duplicated; extract it into
a single shared utility (e.g., export a function named toPascalCase from a
new/shared utils module), replace the local implementations in the agent types
and the rule-identity usage with an import of that exported toPascalCase, remove
the duplicate definitions, and run/typecheck to ensure no signature or behavior
changes; reference the existing symbol name toPascalCase and update any imports
that previously referenced the local implementations.
In `@tests/orchestrator-agent-output.test.ts`:
- Around line 232-237: Tests assert exact progress strings from stderrOutput
(e.g., the REVIEW and LINT messages using README_PATH), which is brittle; change
the test to either reference shared progress format constants defined in the
progress reporter module or relax assertions to semantic regexes: replace the
exact substring checks on stderrOutput with patterns that match the semantic
pieces (e.g., presence of "reviewing", the README_PATH, "calling tool lint", and
"done" timestamps) or import constants exported by the progress reporter
(progress symbols/format strings) and use those constants in the assertions so
the test uses the canonical format instead of hardcoded literals.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: a9d47c1f-8bf3-4526-9c01-c3111bd66c13
📒 Files selected for processing (21)
docs/logs/2026-03-31-agent-mode-implementation-plan.log.mdsrc/agent/agent-executor.tssrc/agent/index.tssrc/agent/merger.tssrc/agent/review-session-store.tssrc/agent/session-replay.tssrc/agent/tools/index.tssrc/agent/tools/lint-tool.tssrc/agent/types.tssrc/cli/commands.tssrc/cli/orchestrator.tssrc/cli/types.tssrc/output/agent-progress-reporter.tssrc/prompts/rule-identity.tssrc/schemas/cli-schemas.tstests/agent/agent-executor.test.tstests/agent/bootstrap.test.tstests/agent/lint-tool.test.tstests/agent/review-session-store.test.tstests/agent/types-redesign.test.tstests/orchestrator-agent-output.test.ts
Comment on lines
+49
to
+61
| return { | ||
| totalFiles: targets.length, | ||
| totalErrors: replayReport.summary.errors, | ||
| totalWarnings: replayReport.summary.warnings, | ||
| requestFailures: 0, | ||
| hadOperationalErrors: Boolean(runResult.error), | ||
| hadSeverityErrors: false, | ||
| tokenUsage: { | ||
| totalInputTokens: 0, | ||
| totalOutputTokens: 0, | ||
| }, | ||
| agentReport: replayReport, | ||
| }; |
There was a problem hiding this comment.
hadSeverityErrors is hardcoded to false, bypassing severity-based exit codes.
Per src/cli/commands.ts:241, the CLI exits with code 1 when hadSeverityErrors is true. With this always set to false, agent mode will never exit non-zero based on finding severity, unlike lint mode. If the replay report contains error-severity findings, this should likely derive from replayReport.summary.errors > 0.
🔧 Proposed fix
return {
totalFiles: targets.length,
totalErrors: replayReport.summary.errors,
totalWarnings: replayReport.summary.warnings,
requestFailures: 0,
hadOperationalErrors: Boolean(runResult.error),
- hadSeverityErrors: false,
+ hadSeverityErrors: replayReport.summary.errors > 0,
tokenUsage: {
totalInputTokens: 0,
totalOutputTokens: 0,
},
agentReport: replayReport,
};📝 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
| return { | |
| totalFiles: targets.length, | |
| totalErrors: replayReport.summary.errors, | |
| totalWarnings: replayReport.summary.warnings, | |
| requestFailures: 0, | |
| hadOperationalErrors: Boolean(runResult.error), | |
| hadSeverityErrors: false, | |
| tokenUsage: { | |
| totalInputTokens: 0, | |
| totalOutputTokens: 0, | |
| }, | |
| agentReport: replayReport, | |
| }; | |
| return { | |
| totalFiles: targets.length, | |
| totalErrors: replayReport.summary.errors, | |
| totalWarnings: replayReport.summary.warnings, | |
| requestFailures: 0, | |
| hadOperationalErrors: Boolean(runResult.error), | |
| hadSeverityErrors: replayReport.summary.errors > 0, | |
| tokenUsage: { | |
| totalInputTokens: 0, | |
| totalOutputTokens: 0, | |
| }, | |
| agentReport: replayReport, | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/agent/index.ts` around lines 49 - 61, The returned report currently
hardcodes hadSeverityErrors to false, preventing agent mode from exiting with a
non-zero code when replay findings have error severity; update the value of
hadSeverityErrors in the object returned by the function to derive from the
replayReport (for example set hadSeverityErrors to a boolean based on
replayReport.summary.errors > 0 or the appropriate severity count) so it
reflects actual error-severity findings from replayReport.summary; keep existing
fields (totalFiles, totalErrors, etc.) unchanged and only replace the hardcoded
false with the computed boolean.
Comment on lines
+111
to
+117
| export function canonicalRuleIdFromSource(ruleSource: string): string { | ||
| const normalized = ruleSource.replace(/\\/g, "/"); | ||
| const parts = normalized.split("/").filter(Boolean); | ||
| const ruleName = toPascalCase(parts.at(-1) ?? "rule"); | ||
| const packName = toPascalCase(parts.at(-2) ?? "default"); | ||
| return `${packName}.${ruleName}`; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for usages of both functions to verify consistent application
echo "=== canonicalRuleIdFromSource usages ==="
rg -n 'canonicalRuleIdFromSource' --type=ts
echo -e "\n=== canonicalRuleIdFromPackRule usages ==="
rg -n 'canonicalRuleIdFromPackRule' --type=tsRepository: TRocket-Labs/vectorlint
Length of output: 769
🏁 Script executed:
#!/bin/bash
# Get the implementation of canonicalRuleIdFromPackRule
echo "=== canonicalRuleIdFromPackRule implementation ==="
sed -n '13,25p' src/prompts/rule-identity.ts
echo -e "\n=== toPascalCase function ==="
rg -A 5 'function toPascalCase\|export.*toPascalCase' --type=ts
echo -e "\n=== canonicalRuleIdFromPackRule call site ==="
sed -n '65,80p' src/agent/agent-executor.ts
echo -e "\n=== Test case for canonicalRuleIdFromSource ==="
sed -n '15,20p' tests/agent/types-redesign.test.tsRepository: TRocket-Labs/vectorlint
Length of output: 1358
🏁 Script executed:
#!/bin/bash
# Search for toPascalCase definition
echo "=== Finding toPascalCase definition ==="
rg -B 2 -A 10 'toPascalCase' --type=ts | head -50
echo -e "\n=== Check where toPascalCase is imported from in types.ts ==="
head -20 src/agent/types.ts
echo -e "\n=== Check where toPascalCase is imported from in rule-identity.ts ==="
head -20 src/prompts/rule-identity.tsRepository: TRocket-Labs/vectorlint
Length of output: 3801
Extract the duplicated toPascalCase function into a shared module.
The same toPascalCase implementation appears in both src/agent/types.ts and src/prompts/rule-identity.ts. This violates the DRY principle and creates maintenance risk if the implementations diverge. Move this utility to a shared location (e.g., src/utils/pascal-case.ts or similar) and import it in both files.
Additionally, canonicalRuleIdFromSource is defined and tested but not used anywhere in production code—consider whether it is actually needed or if canonicalRuleIdFromPackRule should be the canonical approach throughout the codebase.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/agent/types.ts` around lines 111 - 117, Extract the duplicated
toPascalCase implementation into a single shared utility and update callers:
create a new exported function (e.g., toPascalCase) in a common module and
import it into the files that currently define it, then remove the duplicate
implementations from both the file that defines canonicalRuleIdFromSource and
from rule-identity usage; ensure canonicalRuleIdFromSource continues to call the
shared toPascalCase and run/update its tests, and while here evaluate whether
canonicalRuleIdFromSource is referenced anywhere in production code and, if not
needed, consider consolidating callers to use canonicalRuleIdFromPackRule
instead and remove the unused function.
Comment on lines
+13
to
+16
| function compactRulePreview(ruleSource: string): string { | ||
| const compact = ruleSource.length > 48 ? `${ruleSource.slice(0, 48)}` : ruleSource; | ||
| return `${compact}...`; | ||
| } |
There was a problem hiding this comment.
compactRulePreview unconditionally appends "..." even for short strings.
When ruleSource.length <= 48, the string is not truncated, yet "..." is still appended, which incorrectly implies truncation occurred.
🔧 Proposed fix
function compactRulePreview(ruleSource: string): string {
- const compact = ruleSource.length > 48 ? `${ruleSource.slice(0, 48)}` : ruleSource;
- return `${compact}...`;
+ if (ruleSource.length > 48) {
+ return `${ruleSource.slice(0, 48)}...`;
+ }
+ return ruleSource;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/output/agent-progress-reporter.ts` around lines 13 - 16, The
compactRulePreview function always appends "..." even when ruleSource is shorter
than or equal to 48 characters; update compactRulePreview so it only appends
"..." when truncation occurs (i.e., when ruleSource.length > 48). Locate the
compactRulePreview function and change the return logic to return the original
ruleSource unchanged for short inputs, and return the truncated slice plus "..."
only when the input was actually truncated.
| ) { | ||
| suffix = detail ? ` ${detail}` : ""; | ||
| } | ||
| options.write(`└ calling tool ${toolName} tool${suffix}\n`); |
There was a problem hiding this comment.
Typo: "tool" appears twice in output string.
The output reads "calling tool lint tool..." which is grammatically incorrect.
🐛 Proposed fix
- options.write(`└ calling tool ${toolName} tool${suffix}\n`);
+ options.write(`└ calling tool ${toolName}${suffix}\n`);📝 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
| options.write(`└ calling tool ${toolName} tool${suffix}\n`); | |
| options.write(`└ calling tool ${toolName}${suffix}\n`); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/output/agent-progress-reporter.ts` at line 53, The output string in the
progress reporter duplicates the word "tool" (see the call to options.write in
agent-progress-reporter.ts); update the message in that write call to remove the
repeated word so it reads like "└ calling tool lint..." (or "└ calling lint
tool..." depending on intended phrasing) and keep the existing suffix handling
intact; modify the string passed to options.write in the function that writes
the "calling tool" line to use a single "tool" occurrence.
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
Agent mode behavior had drifted and produced fragile completion/reporting semantics. This redesign establishes deterministic, replay-backed findings with explicit finalization so agent runs are reliable, machine-safe across output modes, and easier to reason about operationally.
What
--mode agentand--printCLI wiring.lint(file, ruleSource, context?), top-levelreport_finding, and mandatoryfinalize_review.~/.vectorlint/reviews/*.jsonlwith typed lifecycle/finding events.lintoutput authoritative for inline findings, persisted asfinding_recorded_inlineevents.finalize_reviewis missing and propagated this to orchestrator operational error handling.--printsuppression and JSON-family machine-safe output.tests/agent/*,tests/orchestrator-agent-output.test.ts).Scope
In scope
src/agent/*runtime components and tools.Out of scope
feat/agent) or backport strategy.Behavior impact
finalize_reviewsignal to be considered successful.linttool outputs and persisted immediately.--printsuppresses progress UI.Risk
src/agent/*) and orchestration wiring.How to test / verify
Checks run
npm run lintnpm run buildnpm run test:run -- tests/agent tests/orchestrator-agent-output.test.tsLLM_PROVIDER=openai OPENAI_API_KEY=dummy OPENAI_MODEL=gpt-4o npm run dev -- README.md --mode=agent --config tests/evaluations/.vectorlint.iniLLM_PROVIDER=openai OPENAI_API_KEY=dummy OPENAI_MODEL=gpt-4o npm run dev -- README.md --mode=agent --print --config tests/evaluations/.vectorlint.iniManual verification
vectorlint <file> --mode=agentin a TTY and confirm progress lines follow the expected format and done markers.--printand confirm interactive progress does not appear.--output jsonand confirm stdout is machine-parseable JSON with replay-derived findings/scores.finalize_reviewand confirm non-zero / operational error behavior.Rollback
--mode agentusage paths while retaining existing lint-mode flows.Summary by CodeRabbit
Release Notes
New Features
--modeCLI option to select "lint" (default) or "agent" runtime--printflag to suppress outputTests