feat: add agent reviewer (--review) and bump to v0.4.4#292
Open
rubenmarcus wants to merge 1 commit intomainfrom
Open
feat: add agent reviewer (--review) and bump to v0.4.4#292rubenmarcus wants to merge 1 commit intomainfrom
rubenmarcus wants to merge 1 commit intomainfrom
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
rubenmarcus
force-pushed
the
feat/agent-reviewer
branch
from
ed90fa8 to
b549ad2
Compare
Contributor
✔️ Bundle Size Analysis
Bundle breakdown |
Member
Author
Contributor
Greptile SummaryThis PR introduces an LLM-powered agent reviewer step ( Key changes:
Issues found:
Confidence Score: 3/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant Executor
participant Validation
participant Reviewer
participant Git
participant LLM
User->>Executor: runLoop({ review: true, ... })
loop Each Iteration (i > 1, pastWarmup)
Executor->>Executor: Agent runs task, produces changes
alt validationCommands configured
Executor->>Validation: runAllValidations()
Validation-->>Executor: ValidationResult[]
alt Validation fails
Executor->>Executor: lastValidationFeedback = feedback
Executor->>Executor: continue (next iteration)
else Validation passes
Executor->>Executor: circuitBreaker.recordSuccess()
Executor->>Executor: lastValidationFeedback = ''
end
end
alt options.review && hasChanges
Executor->>Reviewer: runReview(cwd)
Reviewer->>Git: git add --intent-to-add --all
Reviewer->>Git: git diff HEAD
Git-->>Reviewer: diff output
Reviewer->>Git: git restore --staged .
Reviewer->>LLM: tryCallLLM(diff, REVIEW_SYSTEM_PROMPT)
LLM-->>Reviewer: JSON findings[]
Reviewer->>Reviewer: parseFindings()
Reviewer-->>Executor: ReviewResult { passed, findings }
alt reviewResult.passed == false (has errors)
Executor->>Executor: circuitBreaker.recordFailure()
Executor->>Executor: lastValidationFeedback = formatReviewFeedback()
Executor->>Executor: continue (next iteration)
else reviewResult.passed == true
Executor->>Executor: circuitBreaker.recordSuccess()
Executor->>Executor: lastValidationFeedback = ''
end
end
alt options.commit && hasChanges
Executor->>Git: gitCommit()
end
end
Executor-->>User: LoopResult
Prompt To Fix All With AIThis is a comment left during a code review.
Path: src/loop/reviewer.ts
Line: 41-46
Comment:
**`git restore --staged .` unstages genuine pre-existing staged changes**
`git restore --staged .` is designed to clean up the intent-to-add entries, but it indiscriminately unstages **all** staged changes — not just the ones added by `git add --intent-to-add --all`. If a user had previously staged specific files with `git add` (e.g., as part of a partial commit workflow) before kicking off the agent loop, those staged changes are silently unstaged after the reviewer runs.
For example:
1. User runs `git add src/config.ts` to stage a specific file
2. User runs `ralph-starter run "my task" --review`
3. After `getDiff` completes, `src/config.ts` is no longer staged — the user's staging state is destroyed
A safer cleanup would only remove the entries that were actually registered by the intent-to-add step, rather than blanket-resetting the entire index. One approach is to capture the list of untracked files before calling `git add --intent-to-add --all`, and then restore only those files afterward:
```typescript
// Capture untracked files before registering them
const untracked = await execa('git', ['ls-files', '--others', '--exclude-standard'], { cwd, reject: false });
const untrackedFiles = untracked.stdout.split('\n').filter(Boolean);
await execa('git', ['add', '--intent-to-add', '--all'], { cwd, reject: false });
const head = await execa('git', ['diff', 'HEAD'], { cwd, reject: false });
// Clean up: only remove the intent-to-add entries we added, preserving other staged changes
if (untrackedFiles.length > 0) {
await execa('git', ['restore', '--staged', '--', ...untrackedFiles], { cwd, reject: false });
}
```
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: src/loop/executor.ts
Line: 1514-1515
Comment:
**Reviewer LLM cost not counted by `costTracker`**
`runReview` calls `tryCallLLM` internally, but this additional LLM call is invisible to the `costTracker`. When a user sets `maxCost` to cap spending, reviewer calls are silently excluded from the budget. In a long-running loop with many iterations, the reviewer can make as many LLM calls as there are iterations, potentially pushing real spend significantly above the stated limit.
Consider passing a cost-tracking callback or the `CostTracker` instance into `runReview` so reviewer token usage is accounted for:
```typescript
const reviewResult = await runReview(options.cwd, costTracker ?? undefined);
```
And in `reviewer.ts`:
```typescript
export async function runReview(cwd: string, costTracker?: CostTracker): Promise<ReviewResult | null> {
// ...
const response = await tryCallLLM({ ... });
if (response && costTracker) {
costTracker.recordUsage(response); // track reviewer tokens
}
// ...
}
```
How can I resolve this? If you propose a fix, please make it concise.Last reviewed commit: e1f9ef7 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
rubenmarcus
force-pushed
the
feat/agent-reviewer
branch
2 times, most recently
from
65a2d98 to
1f06989
Compare
Member
Author
rubenmarcus
force-pushed
the
feat/agent-reviewer
branch
from
1f06989 to
40b306d
Compare
Member
Author
Comment on lines
+1543
to
+1551
| if (reviewResult) { | ||
| const warnCount = reviewResult.findings.filter((f) => f.severity === 'warning').length; | ||
| const suffix = warnCount > 0 ? ` (${warnCount} warning(s))` : ''; | ||
| spinner.succeed(chalk.green(`Loop ${i}: Agent review passed${suffix}`)); | ||
| circuitBreaker.recordSuccess(); | ||
| lastValidationFeedback = ''; | ||
| } else { | ||
| spinner.info(chalk.dim(`Loop ${i}: Agent review skipped (no diff or no LLM key)`)); | ||
| } |
Contributor
There was a problem hiding this comment.
Warning findings are silently swallowed when review passes
When the agent review passes but contains warning-severity findings, only the count is shown in the spinner suffix (e.g., "Agent review passed (2 warning(s))"). The actual warning messages are never logged to the console and are never fed back to the agent via lastValidationFeedback (which is correctly cleared to ''). This means the user and the agent have no visibility into the content of the warnings.
Compare this to the failure path (lines 1528–1531) where every finding is individually logged. Warnings should receive the same treatment so the operator can see and act on them:
Suggested change
| if (reviewResult) { | |
| const warnCount = reviewResult.findings.filter((f) => f.severity === 'warning').length; | |
| const suffix = warnCount > 0 ? ` (${warnCount} warning(s))` : ''; | |
| spinner.succeed(chalk.green(`Loop ${i}: Agent review passed${suffix}`)); | |
| circuitBreaker.recordSuccess(); | |
| lastValidationFeedback = ''; | |
| } else { | |
| spinner.info(chalk.dim(`Loop ${i}: Agent review skipped (no diff or no LLM key)`)); | |
| } | |
| if (reviewResult) { | |
| const warnFindings = reviewResult.findings.filter((f) => f.severity === 'warning'); | |
| const warnCount = warnFindings.length; | |
| const suffix = warnCount > 0 ? ` (${warnCount} warning(s))` : ''; | |
| spinner.succeed(chalk.green(`Loop ${i}: Agent review passed${suffix}`)); | |
| for (const f of warnFindings) { | |
| log(chalk.dim(` ⚠️ ${f.message}`)); | |
| } | |
| circuitBreaker.recordSuccess(); | |
| lastValidationFeedback = ''; | |
| } |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/loop/executor.ts
Line: 1543-1551
Comment:
**Warning findings are silently swallowed when review passes**
When the agent review passes but contains `warning`-severity findings, only the count is shown in the spinner suffix (e.g., `"Agent review passed (2 warning(s))"`). The actual warning messages are never logged to the console and are never fed back to the agent via `lastValidationFeedback` (which is correctly cleared to `''`). This means the user and the agent have no visibility into the content of the warnings.
Compare this to the failure path (lines 1528–1531) where every finding is individually logged. Warnings should receive the same treatment so the operator can see and act on them:
```suggestion
if (reviewResult) {
const warnFindings = reviewResult.findings.filter((f) => f.severity === 'warning');
const warnCount = warnFindings.length;
const suffix = warnCount > 0 ? ` (${warnCount} warning(s))` : '';
spinner.succeed(chalk.green(`Loop ${i}: Agent review passed${suffix}`));
for (const f of warnFindings) {
log(chalk.dim(` ⚠️ ${f.message}`));
}
circuitBreaker.recordSuccess();
lastValidationFeedback = '';
}
```
How can I resolve this? If you propose a fix, please make it concise.
rubenmarcus
force-pushed
the
feat/agent-reviewer
branch
from
40b306d to
282f255
Compare
Member
Author
rubenmarcus
force-pushed
the
feat/agent-reviewer
branch
from
282f255 to
82c7706
Compare
Member
Author
rubenmarcus
force-pushed
the
feat/agent-reviewer
branch
from
82c7706 to
0750d3c
Compare
Member
Author
rubenmarcus
force-pushed
the
feat/agent-reviewer
branch
from
0750d3c to
695ec81
Compare
Member
Author
rubenmarcus
force-pushed
the
feat/agent-reviewer
branch
from
695ec81 to
5d76d7d
Compare
Member
Author
- Add LLM-powered diff review step that runs after lint/build/test pass but before commit, catching security issues, logic errors, and pattern violations that automated checks miss - New src/loop/reviewer.ts module using existing tryCallLLM infrastructure (Anthropic/OpenAI/OpenRouter) with structured JSON findings output - Errors block commit and feed back via lastValidationFeedback; warnings are logged but non-blocking; gracefully skips if no diff or no API key - Wire --review CLI flag on run command and export public API types - Close 14 delivered issues (#212, #224, #225, #226, #227, #228, #229, #231, #232, #233, #237, #239, #240, #241) - Bump version to 0.4.4 Co-authored-by: Amp <amp@ampcode.com> Amp-Thread-ID: https://ampcode.com/threads/T-019ce2d4-0f5f-742e-9721-3181f57267df
rubenmarcus
force-pushed
the
feat/agent-reviewer
branch
from
5d76d7d to
e1f9ef7
Compare
Member
Author
Comment on lines
+41
to
+46
| await execa('git', ['add', '--intent-to-add', '--all'], { cwd, reject: false }); | ||
|
|
||
| const head = await execa('git', ['diff', 'HEAD'], { cwd, reject: false }); | ||
|
|
||
| // Clean up: remove intent-to-add entries so the index isn't permanently mutated | ||
| await execa('git', ['restore', '--staged', '.'], { cwd, reject: false }); |
Contributor
There was a problem hiding this comment.
git restore --staged . unstages genuine pre-existing staged changes
git restore --staged . is designed to clean up the intent-to-add entries, but it indiscriminately unstages all staged changes — not just the ones added by git add --intent-to-add --all. If a user had previously staged specific files with git add (e.g., as part of a partial commit workflow) before kicking off the agent loop, those staged changes are silently unstaged after the reviewer runs.
For example:
- User runs
git add src/config.tsto stage a specific file - User runs
ralph-starter run "my task" --review - After
getDiffcompletes,src/config.tsis no longer staged — the user's staging state is destroyed
A safer cleanup would only remove the entries that were actually registered by the intent-to-add step, rather than blanket-resetting the entire index. One approach is to capture the list of untracked files before calling git add --intent-to-add --all, and then restore only those files afterward:
// Capture untracked files before registering them
const untracked = await execa('git', ['ls-files', '--others', '--exclude-standard'], { cwd, reject: false });
const untrackedFiles = untracked.stdout.split('\n').filter(Boolean);
await execa('git', ['add', '--intent-to-add', '--all'], { cwd, reject: false });
const head = await execa('git', ['diff', 'HEAD'], { cwd, reject: false });
// Clean up: only remove the intent-to-add entries we added, preserving other staged changes
if (untrackedFiles.length > 0) {
await execa('git', ['restore', '--staged', '--', ...untrackedFiles], { cwd, reject: false });
}Prompt To Fix With AI
This is a comment left during a code review.
Path: src/loop/reviewer.ts
Line: 41-46
Comment:
**`git restore --staged .` unstages genuine pre-existing staged changes**
`git restore --staged .` is designed to clean up the intent-to-add entries, but it indiscriminately unstages **all** staged changes — not just the ones added by `git add --intent-to-add --all`. If a user had previously staged specific files with `git add` (e.g., as part of a partial commit workflow) before kicking off the agent loop, those staged changes are silently unstaged after the reviewer runs.
For example:
1. User runs `git add src/config.ts` to stage a specific file
2. User runs `ralph-starter run "my task" --review`
3. After `getDiff` completes, `src/config.ts` is no longer staged — the user's staging state is destroyed
A safer cleanup would only remove the entries that were actually registered by the intent-to-add step, rather than blanket-resetting the entire index. One approach is to capture the list of untracked files before calling `git add --intent-to-add --all`, and then restore only those files afterward:
```typescript
// Capture untracked files before registering them
const untracked = await execa('git', ['ls-files', '--others', '--exclude-standard'], { cwd, reject: false });
const untrackedFiles = untracked.stdout.split('\n').filter(Boolean);
await execa('git', ['add', '--intent-to-add', '--all'], { cwd, reject: false });
const head = await execa('git', ['diff', 'HEAD'], { cwd, reject: false });
// Clean up: only remove the intent-to-add entries we added, preserving other staged changes
if (untrackedFiles.length > 0) {
await execa('git', ['restore', '--staged', '--', ...untrackedFiles], { cwd, reject: false });
}
```
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+1514
to
+1515
| // --- Agent reviewer: LLM-powered diff review before commit --- | ||
| if (options.review && hasChanges && i > 1 && pastWarmup) { |
Contributor
There was a problem hiding this comment.
Reviewer LLM cost not counted by costTracker
runReview calls tryCallLLM internally, but this additional LLM call is invisible to the costTracker. When a user sets maxCost to cap spending, reviewer calls are silently excluded from the budget. In a long-running loop with many iterations, the reviewer can make as many LLM calls as there are iterations, potentially pushing real spend significantly above the stated limit.
Consider passing a cost-tracking callback or the CostTracker instance into runReview so reviewer token usage is accounted for:
const reviewResult = await runReview(options.cwd, costTracker ?? undefined);And in reviewer.ts:
export async function runReview(cwd: string, costTracker?: CostTracker): Promise<ReviewResult | null> {
// ...
const response = await tryCallLLM({ ... });
if (response && costTracker) {
costTracker.recordUsage(response); // track reviewer tokens
}
// ...
}Prompt To Fix With AI
This is a comment left during a code review.
Path: src/loop/executor.ts
Line: 1514-1515
Comment:
**Reviewer LLM cost not counted by `costTracker`**
`runReview` calls `tryCallLLM` internally, but this additional LLM call is invisible to the `costTracker`. When a user sets `maxCost` to cap spending, reviewer calls are silently excluded from the budget. In a long-running loop with many iterations, the reviewer can make as many LLM calls as there are iterations, potentially pushing real spend significantly above the stated limit.
Consider passing a cost-tracking callback or the `CostTracker` instance into `runReview` so reviewer token usage is accounted for:
```typescript
const reviewResult = await runReview(options.cwd, costTracker ?? undefined);
```
And in `reviewer.ts`:
```typescript
export async function runReview(cwd: string, costTracker?: CostTracker): Promise<ReviewResult | null> {
// ...
const response = await tryCallLLM({ ... });
if (response && costTracker) {
costTracker.recordUsage(response); // track reviewer tokens
}
// ...
}
```
How can I resolve this? If you propose a fix, please make it concise.
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.
Summary
Adds an LLM-powered agent reviewer step to the executor loop and prepares the v0.4.4 release.
Agent Reviewer (
--review)A new validation step that runs after lint/build/test pass but before commit. It analyzes the git diff with an LLM to catch issues that automated checks miss:
Usage:
ralph-starter run "my task" --reviewFindings with
severity: errorblock the commit and feed back into the next iteration vialastValidationFeedback. Warnings are logged but non-blocking. Gracefully skips if no diff exists or no LLM API key is available.Issue Cleanup
Closed 14 delivered issues that were still marked as open:
Open issues reduced from 19 → 5 (all genuine future work).
Files Changed
src/loop/reviewer.ts— new module (reviewer logic, LLM prompt, JSON parsing)src/loop/executor.ts— integrate reviewer step after validation, before commitsrc/cli.ts— add--reviewflag toruncommandsrc/commands/run.ts— wirereviewoption through toLoopOptionssrc/index.ts— export public API types (ReviewResult,runReview, etc.)package.json— bump version to 0.4.4