Skip to content
Merged
Show file tree
Hide file tree
Changes from 16 commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
9b0bdf8
feat(core): enforce prior read before Edit / WriteFile mutates a file
wenshao Apr 30, 2026
3c78a1f
test(core): clear shared fileReadCache between write-file.test.ts cases
wenshao Apr 30, 2026
3b79cfc
fix(core): close prior-read enforcement gaps flagged in 3rd review
wenshao Apr 30, 2026
303b6b7
refactor(core): close 4 prior-read enforcement gaps from 4th review
wenshao May 1, 2026
d6ffe80
refactor(core): close 4 prior-read enforcement gaps from 4th review (…
wenshao May 2, 2026
248d179
refactor(core): tighten StructuredToolError naming + add scheduler test
wenshao May 3, 2026
323e080
fix(core): close TOCTOU + grammar + directory regressions in PR-B
wenshao May 3, 2026
f4ef756
fix(core): close fileExists TOCTOU on WriteFile prior-read enforcement
wenshao May 3, 2026
a9ec1a8
fix(core): close 4 enforcement gaps + 1 critical bug from 5th Copilot…
wenshao May 3, 2026
fa9344c
fix(core): close new-file race + special-file enforcement loop
wenshao May 3, 2026
9df900a
fix(core): close 4 enforcement gaps from 6th Copilot review
wenshao May 3, 2026
4482dd5
Merge remote-tracking branch 'origin/main' into feat/edit-write-prior…
wenshao May 4, 2026
b053ae3
docs(core): clarify stat→write race + EDIT_REQUIRES_PRIOR_READ scope
wenshao May 4, 2026
40b7a11
Merge remote-tracking branch 'origin/main' into feat/edit-write-prior…
wenshao May 4, 2026
9726c87
chore(core): minor follow-ups from review #4229917446
wenshao May 5, 2026
de8ddf5
fix(core): close 2 correctness gaps from maintainer review #4232751470
wenshao May 6, 2026
72a1d67
fix(core): close 3 correctness gaps from re-review #4233904930
wenshao May 6, 2026
70f7223
docs(core): clarify partial subagent isolation per review #4234090906
wenshao May 6, 2026
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
Loading
Loading