Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 14 additions & 14 deletions docs/users/features/code-review.md
Original file line number Diff line number Diff line change
Expand Up @@ -129,15 +129,15 @@ You can review PRs from other repositories by passing the full URL:

This runs in **lightweight mode** — no worktree, no linter, no build/test, no autofix. The review is based on the diff text only (fetched via GitHub API). PR comments can still be posted if you have write access.

| Capability | Same-repo | Cross-repo |
| ------------------------------------------------ | --------- | ----------------------------- |
| Capability | Same-repo | Cross-repo |
| ---------------------------------------------------------- | --------- | ----------------------------- |
| LLM review (Agents 1-6 + verify + iterative reverse audit) | ✅ | ✅ |
| Agent 7: Build & test | ✅ | ❌ (no local codebase) |
| Deterministic analysis (linter/typecheck) | ✅ | ❌ |
| Cross-file impact analysis | ✅ | ❌ |
| Autofix | ✅ | ❌ |
| PR inline comments | ✅ | ✅ (if you have write access) |
| Incremental review cache | ✅ | ❌ |
| Deterministic analysis (linter/typecheck) | ✅ | ❌ |
| Cross-file impact analysis | ✅ | ❌ |
| Autofix | ✅ | ❌ |
| PR inline comments | ✅ | ✅ (if you have write access) |
| Incremental review cache | ✅ | ❌ |

## PR Inline Comments

Expand Down Expand Up @@ -258,13 +258,13 @@ For large diffs (>10 modified symbols), analysis prioritizes functions with sign

The review pipeline uses a bounded number of LLM calls regardless of how many findings are produced:

| Stage | LLM calls | Notes |
| -------------------------------- | ----------------- | ---------------------------------------------------- |
| Deterministic analysis (Step 3) | 0 | Shell commands only |
| Review agents (Step 4) | 9 (or 8) | Run in parallel; Agent 7 skipped in cross-repo mode |
| Batch verification (Step 5) | 1 | Single agent verifies all findings at once |
| Iterative reverse audit (Step 6) | 1-3 | Loops until "No issues found" or 3-round cap |
| **Total** | **11-13 (10-12)** | Same-repo: 11-13; cross-repo: 10-12 (no Agent 7) |
| Stage | LLM calls | Notes |
| -------------------------------- | ----------------- | --------------------------------------------------- |
| Deterministic analysis (Step 3) | 0 | Shell commands only |
| Review agents (Step 4) | 9 (or 8) | Run in parallel; Agent 7 skipped in cross-repo mode |
| Batch verification (Step 5) | 1 | Single agent verifies all findings at once |
| Iterative reverse audit (Step 6) | 1-3 | Loops until "No issues found" or 3-round cap |
| **Total** | **11-13 (10-12)** | Same-repo: 11-13; cross-repo: 10-12 (no Agent 7) |

Most PRs converge to the lower end of the range (1 reverse audit round); the cap prevents runaway cost on pathological cases.

Expand Down
4 changes: 1 addition & 3 deletions packages/cli/src/commands/review/cleanup.ts
Original file line number Diff line number Diff line change
Expand Up @@ -84,9 +84,7 @@ function runCleanup(target: string): void {
writeStdoutLine(`Removed temp file: ${full}`);
removedAny = true;
} catch (err) {
writeStderrLine(
`Failed to remove ${full}: ${(err as Error).message}`,
);
writeStderrLine(`Failed to remove ${full}: ${(err as Error).message}`);
}
}

Expand Down
25 changes: 17 additions & 8 deletions packages/cli/src/commands/review/deterministic.ts
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,10 @@
// review output verbatim — they skip Step 5 verification.

import type { CommandModule } from 'yargs';
import { execFileSync, type ExecFileSyncOptionsWithStringEncoding } from 'node:child_process';
import {
execFileSync,
type ExecFileSyncOptionsWithStringEncoding,
} from 'node:child_process';
import { existsSync, readFileSync, writeFileSync, mkdirSync } from 'node:fs';
import { join, dirname, resolve } from 'node:path';
import { writeStdoutLine } from '../../utils/stdioHelpers.js';
Expand Down Expand Up @@ -279,7 +282,11 @@ const eslintTool: ToolDef = {
],
TIMEOUT_LINTER_MS,
);
const findings = parseEslintJson(ex.stdout, ctx.worktree, ctx.changedFilesSet);
const findings = parseEslintJson(
ex.stdout,
ctx.worktree,
ctx.changedFilesSet,
);
return { exitCode: ex.exitCode, findings, timedOut: ex.timedOut };
},
};
Expand Down Expand Up @@ -351,7 +358,8 @@ const ruffTool: ToolDef = {
if (!hasConfig) {
return {
ok: false,
reason: 'no ruff config (ruff.toml / .ruff.toml / pyproject [tool.ruff])',
reason:
'no ruff config (ruff.toml / .ruff.toml / pyproject [tool.ruff])',
};
}
if (!which('ruff')) return { ok: false, reason: 'ruff not in PATH' };
Expand All @@ -368,7 +376,11 @@ const ruffTool: ToolDef = {
['check', '--output-format=json', ...targets],
TIMEOUT_LINTER_MS,
);
const findings = parseRuffJson(ex.stdout, ctx.worktree, ctx.changedFilesSet);
const findings = parseRuffJson(
ex.stdout,
ctx.worktree,
ctx.changedFilesSet,
);
return { exitCode: ex.exitCode, findings, timedOut: ex.timedOut };
},
};
Expand Down Expand Up @@ -695,10 +707,7 @@ async function runDeterministic(args: DeterministicArgs): Promise<void> {
writeFileSync(args.out, JSON.stringify(result, null, 2) + '\n', 'utf8');

const summary = toolsRun
.map(
(r) =>
`${r.tool}=${r.findingsCount}${r.timedOut ? ' (timeout)' : ''}`,
)
.map((r) => `${r.tool}=${r.findingsCount}${r.timedOut ? ' (timeout)' : ''}`)
.join(', ');
writeStdoutLine(
`Wrote deterministic report to ${args.out}: ${findings.length} findings (${summary || 'no tools applicable'}; skipped ${toolsSkipped.length})`,
Expand Down
13 changes: 2 additions & 11 deletions packages/cli/src/commands/review/fetch-pr.ts
Original file line number Diff line number Diff line change
Expand Up @@ -26,11 +26,7 @@ import { dirname } from 'node:path';
import { writeStdoutLine, writeStderrLine } from '../../utils/stdioHelpers.js';
import { ensureAuthenticated, gh } from './lib/gh.js';
import { git, refExists } from './lib/git.js';
import {
REVIEW_TMP_DIR,
reviewBranch,
worktreePath,
} from './lib/paths.js';
import { REVIEW_TMP_DIR, reviewBranch, worktreePath } from './lib/paths.js';

interface PrMetadata {
headRefName: string;
Expand Down Expand Up @@ -88,12 +84,7 @@ function cleanStale(prNumber: string): void {
}

async function runFetchPr(args: FetchPrArgs): Promise<void> {
const {
pr_number: prNumber,
owner_repo: ownerRepo,
remote,
out,
} = args;
const { pr_number: prNumber, owner_repo: ownerRepo, remote, out } = args;

if (ownerRepo.indexOf('/') < 0) {
throw new Error('owner_repo must look like "owner/repo"');
Expand Down
9 changes: 6 additions & 3 deletions packages/cli/src/commands/review/load-rules.ts
Original file line number Diff line number Diff line change
Expand Up @@ -123,7 +123,9 @@ async function runLoadRules(args: LoadRulesArgs): Promise<void> {
writeFileSync(out, combined, 'utf8');

if (loaded.length === 0) {
writeStdoutLine(`No review rules found on ${baseRef}; wrote empty file to ${out}`);
writeStdoutLine(
`No review rules found on ${baseRef}; wrote empty file to ${out}`,
);
} else {
writeStdoutLine(
`Loaded ${loaded.length} rule source(s) from ${baseRef} → ${out}: ${loaded.join(', ')}`,
Expand All @@ -134,7 +136,7 @@ async function runLoadRules(args: LoadRulesArgs): Promise<void> {
export const loadRulesCommand: CommandModule = {
command: 'load-rules <base_ref>',
describe:
"Read project review rules from the base branch (.qwen/review-rules.md, .github/copilot-instructions.md, AGENTS.md, QWEN.md) and write a combined Markdown file",
'Read project review rules from the base branch (.qwen/review-rules.md, .github/copilot-instructions.md, AGENTS.md, QWEN.md) and write a combined Markdown file',
builder: (yargs) =>
yargs
.positional('base_ref', {
Expand All @@ -146,7 +148,8 @@ export const loadRulesCommand: CommandModule = {
.option('out', {
type: 'string',
demandOption: true,
describe: 'Output Markdown path (will be overwritten — empty if no rules found)',
describe:
'Output Markdown path (will be overwritten — empty if no rules found)',
}),
handler: async (argv) => {
await runLoadRules(argv as unknown as LoadRulesArgs);
Expand Down
25 changes: 12 additions & 13 deletions packages/cli/src/commands/review/pr-context.ts
Original file line number Diff line number Diff line change
Expand Up @@ -67,10 +67,7 @@ function snippet(s: string | undefined, max = 240): string {
* Walk a comment's `in_reply_to_id` chain up to the root. Defends against
* cycles (which shouldn't happen on GitHub but cheap to handle).
*/
function findRootId(
startId: number,
byId: Map<number, RawComment>,
): number {
function findRootId(startId: number, byId: Map<number, RawComment>): number {
const seen = new Set<number>();
let cur = startId;
while (true) {
Expand Down Expand Up @@ -141,7 +138,9 @@ function buildMarkdown(
parts.push(`- **Repo:** ${ownerRepo}`);
parts.push(`- **Author:** @${meta.author?.login ?? 'unknown'}`);
parts.push(`- **State:** ${meta.state}`);
parts.push(`- **Base → Head:** \`${meta.baseRefName}\` ← \`${meta.headRefName}\``);
parts.push(
`- **Base → Head:** \`${meta.baseRefName}\` ← \`${meta.headRefName}\``,
);
parts.push(`- **HEAD SHA:** \`${meta.headRefOid}\``);
parts.push(
`- **Diff:** ${meta.changedFiles} files, +${meta.additions}/-${meta.deletions}`,
Expand Down Expand Up @@ -187,7 +186,9 @@ function buildMarkdown(
// only root-comment snippets and forced the LLM driver to manually
// summarise each reply chain in agent prompts.
if (repliedRoots.length > 0 || issue.length > 0) {
parts.push('## Already discussed — do NOT re-report unless the latest reply itself raises a new concern');
parts.push(
'## Already discussed — do NOT re-report unless the latest reply itself raises a new concern',
);
parts.push('');
if (repliedRoots.length > 0) {
parts.push('### Inline-comment threads with replies');
Expand All @@ -209,9 +210,7 @@ function buildMarkdown(
if (replies.length > 0) {
parts.push('Replies (chronological):');
for (const r of replies) {
parts.push(
`- **@${r.user?.login ?? '?'}**: ${snippet(r.body)}`,
);
parts.push(`- **@${r.user?.login ?? '?'}**: ${snippet(r.body)}`);
}
parts.push('');
}
Expand All @@ -221,16 +220,16 @@ function buildMarkdown(
parts.push('### Issue-level comments (general PR thread)');
parts.push('');
for (const c of issue) {
parts.push(
`- by @${c.user?.login ?? '?'}: ${snippet(c.body)}`,
);
parts.push(`- by @${c.user?.login ?? '?'}: ${snippet(c.body)}`);
}
parts.push('');
}
}

if (openRoots.length > 0) {
parts.push('## Open inline comments (no replies yet — may still need attention)');
parts.push(
'## Open inline comments (no replies yet — may still need attention)',
);
parts.push('');
for (const c of openRoots) {
parts.push(
Expand Down
8 changes: 4 additions & 4 deletions packages/cli/src/commands/review/presubmit.ts
Original file line number Diff line number Diff line change
Expand Up @@ -201,9 +201,7 @@ async function runPresubmit(args: PresubmitArgs): Promise<void> {
if (newFindingsPath) {
newFindings = JSON.parse(readFileSync(newFindingsPath, 'utf8'));
}
const newFindingKeys = new Set(
newFindings.map((f) => `${f.path}:${f.line}`),
);
const newFindingKeys = new Set(newFindings.map((f) => `${f.path}:${f.line}`));

const buckets = classifyExistingComments(
qwenComments,
Expand All @@ -216,7 +214,9 @@ async function runPresubmit(args: PresubmitArgs): Promise<void> {
const downgradeReasons: string[] = [];
if (isSelfPr) downgradeReasons.push('self-PR');
if (ciStatus.class === 'any_failure') {
downgradeReasons.push(`CI failing: ${ciStatus.failedCheckNames.join(', ')}`);
downgradeReasons.push(
`CI failing: ${ciStatus.failedCheckNames.join(', ')}`,
);
}
if (ciStatus.class === 'all_pending') {
downgradeReasons.push('CI still running');
Expand Down
20 changes: 19 additions & 1 deletion packages/core/src/agents/runtime/agent-core.ts
Original file line number Diff line number Diff line change
Expand Up @@ -317,11 +317,17 @@ export class AgentCore {
}

try {
return new GeminiChat(
const chat = new GeminiChat(
this.runtimeContext,
generationConfig,
startHistory,
);
// Seed the per-chat token count so the auto-compaction threshold
// gate sees the inherited history's true size on the first send.
// Without this, fork subagents start at 0 and the gate NOOPs even
// when `startHistory` is already huge — first API call can 400.
chat.setLastPromptTokenCount(this.lastPromptTokenCount);
return chat;
} catch (error) {
await reportError(
error,
Expand Down Expand Up @@ -540,6 +546,18 @@ export class AgentCore {
continue;
}

// GeminiChat already mutated its own history; surface to the debug
// log so subagent compactions show up alongside the main session's.
if (streamEvent.type === 'compressed') {
this.runtimeContext

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Critical] Subagent lastPromptTokenCount never seeded — auto-compression silently skipped on first send.

GeminiChat.setLastPromptTokenCount() is designed for "chats created with inherited history (forks, subagents, speculation)" per its JSDoc, but createChat() never calls it. As a result lastPromptTokenCount defaults to 0, the threshold check 0 < threshold * contextWindow always passes, and tryCompress returns NOOP on the first sendMessageStream. For fork subagents with large inherited history, the first API call can 400.

This debug log (the only subagent compression surface) will never fire because compression never triggers. AgentCore already tracks lastPromptTokenCount (field at L216); seed it in createChat():

Suggested change
this.runtimeContext
// In createChat(), after `new GeminiChat(...)` returns:
chat.setLastPromptTokenCount(this.lastPromptTokenCount);
return chat;

— deepseek-v4-pro via Qwen Code /review

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 3239bbecreateChat now seeds the new chat with this.lastPromptTokenCount so the threshold gate sees the inherited size on first send. Agreed the realistic blast radius is small (the 40-entry truncation in forkedAgent.ts defangs the fork case in practice), but a one-line seed is cheap insurance against future call sites that pass large extraHistory/initialMessages.

.getDebugLogger()
.debug(
`[AGENT-COMPACT] subagent=${this.subagentId} round=${turnCounter} ` +
`tokens ${streamEvent.info.originalTokenCount} -> ${streamEvent.info.newTokenCount}`,
);
continue;
}

// Handle chunk events
if (streamEvent.type === 'chunk') {
const resp = streamEvent.value;
Expand Down
4 changes: 4 additions & 0 deletions packages/core/src/agents/runtime/agent-headless.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -280,6 +280,7 @@ describe('subagent.ts', () => {
() =>
({
sendMessageStream: mockSendMessageStream,
setLastPromptTokenCount: vi.fn(),
}) as unknown as GeminiChat,
);

Expand Down Expand Up @@ -958,6 +959,7 @@ describe('subagent.ts', () => {
() =>
({
sendMessageStream: mockSendMessageStream,
setLastPromptTokenCount: vi.fn(),
}) as unknown as GeminiChat,
);

Expand Down Expand Up @@ -997,6 +999,7 @@ describe('subagent.ts', () => {
() =>
({
sendMessageStream: mockSendMessageStream,
setLastPromptTokenCount: vi.fn(),
}) as unknown as GeminiChat,
);

Expand Down Expand Up @@ -1061,6 +1064,7 @@ describe('subagent.ts', () => {
() =>
({
sendMessageStream: mockSendMessageStream,
setLastPromptTokenCount: vi.fn(),
}) as unknown as GeminiChat,
);

Expand Down
Loading
Loading