diff --git a/docs/design/review-repository-context.md b/docs/design/review-repository-context.md new file mode 100644 index 00000000000..57c7ece6d1d --- /dev/null +++ b/docs/design/review-repository-context.md @@ -0,0 +1,52 @@ +# Review repository context + +## Problem + +The review pipeline needs a bounded way for repositories to declare review guidance without teaching shared roster, prompt, coverage, and composition code about individual projects. Repository metadata is security-sensitive for pull request reviews because the reviewed branch must not be able to opt into or remove trusted context. + +## Manifest + +A repository may provide strict JSON at `.qwen/review-context.json`: + +```json +{ + "version": 1, + "label": "Example repository", + "rules": [ + { + "paths": ["packages/*/src/**"], + "relatedPaths": ["packages/cli/src/commands/review/**"], + "domains": ["runtime"], + "recommendedTests": ["test:runtime"], + "requiredConfigurations": ["debug"], + "requiredAgents": ["test-matrix"], + "unverifiedDimensions": ["Alternate configuration"], + "verificationNotes": ["Run the repository-native focused tests"] + } + ] +} +``` + +The top-level fields are exactly `version`, `label`, and `rules`. Each rule requires `paths`; all other rule fields are optional. Unknown or missing required fields, comments, unsupported versions, oversized values, control characters, and duplicate array entries are rejected. Arrays are human-authored and may be written in any order; values from all matching rules are merged, deduplicated, and returned sorted and unique (the internal wire format keeps the strict sorted-and-unique check). Rule order is preserved. The total `paths` globs across all rules, the merged `relatedPaths` glob list, and every merged field are capped at the wire bounds and rejected fail-closed, so a matching burst cannot stall the step or outgrow the contract. Note the example's `relatedPaths` wildcard is scoped to one subsystem on purpose: wildcard `relatedPaths` are subject to the 128 resolved-file bound below, and a repository-wide scope like `packages/*/src/**` exceeds it on a repository this size. + +`paths` and `relatedPaths` use repository-relative `/`-separated globs. Matching is case-sensitive on every platform and `?` consumes one UTF-16 code unit. The supported metacharacters are `*`, `?`, and a complete `**` path segment. Absolute paths, backslashes, empty or `.`/`..` segments, negation, brace expansion, character classes, and extended glob syntax are rejected. + +A rule matches when any changed path matches one of its `paths` globs. If no rule matches, the provider returns no context. A matching rule's deduplicated `relatedPaths` globs are expanded from the worktree with dot files enabled, directory results disabled, symlink traversal disabled, and case-sensitive matching. Related globs containing wildcards must start with a non-wildcard directory segment so expansion cannot begin with a repository-wide wildcard; a completely static entry resolves to itself when it exists as a regular file. Globs whose path enters a dependency or build-output directory at any depth are rejected at validation (compared case-insensitively, on every platform), so the never-descend invariant holds for scan roots as well as recursion. Changed paths are removed from the result. Resolved files must remain inside the worktree. Expansion never descends into dependency and build-output trees (`node_modules`, `dist`, and the other conventional names the scan skips) and fails closed when any limit is exceeded: 16384 visited entries across the scan (files and directories, matching or not — calibrated on this repository's installed checkout, so a honestly scoped subtree, including all of `packages/`, never trips it), 128 resolved files in the result, and a matching-work budget charged per attempted pattern match (pattern length times path length) in both the rule filter and the expansion, which reports the matching-work limit and keeps a matching burst from stalling the step. + +## Trust boundary + +`repo-context` reads the fixed manifest path through `RepositoryContextProviderInput.readIdentityFile`. For pull request plans, the manifest therefore comes only from the trusted merge-base commit recorded by the fetch stage. The pull request head cannot opt in, opt out, or change the rules. For local plans, the manifest comes from the current worktree after safe-relative-path validation and realpath containment. + +Identity reads return the same shape in both modes (CRLF normalised to LF, surrounding whitespace trimmed) and are capped at one megabyte, fail closed: an absent file yields `null`, but a file that is present, unreadable, or oversized throws rather than masquerading as "not this repository". Both modes follow a symlinked identity file itself under the same containment rule, and a directory yields nothing in both. A pull request plan whose merge base never resolved (`mergeBaseSha: null`) — or whose base fetch failed, leaving the recorded sha possibly stale — writes a `null` artifact without consulting the worktree at all: falling back to the worktree would read the manifest from the PR head, the exact read this boundary forbids, and a possibly stale sha is not a trusted source either. + +Three residuals are recorded so the guarantee is not overstated. First, for pull request plans the RULES come from the merge base, but `relatedPaths` globs are expanded against the head worktree, so the head still decides which files the base's globs resolve to; impact is low because reviewers read the head tree anyway. Second, local reviews read the manifest from the current worktree, so reviewing an untrusted repository lets that repository put one bounded, control-character-free block of guidance — the label plus six capped arrays — into every code-reviewing brief; the one-megabyte read ceiling, the validation bounds, and inert rendering are the mitigation. Third, the two modes resolve identity symlinks with different engines: the pull request reader never descends through a symlinked intermediate path COMPONENT (the worktree reader does), and it caps identity symlink chains at 16 hops where the kernel resolves up to ~40 — throwing at the cap rather than degrading. A repository committing `.qwen` itself as a symlink to an in-tree directory therefore attaches context in local reviews and never in pull request reviews; the direction is fail-safe (strictly less, never more), but an operator diagnosing "context attaches locally but never on PRs" should know the asymmetry is by construction. + +The manifest provider is statically registered in-process and returns the generic `RepositoryContext` shape with provider `manifest`. Its complete output passes through the shared `validateRepositoryContext` validator before downstream consumers use it. No dynamic plugin registry, shell execution, templates, or opaque payloads are supported. + +## Review workflow + +Medium- and high-effort local and same-repository pull request reviews invoke `repo-context` after the review plan is captured. The command receives absolute plan, worktree, and output paths. Low-effort reviews and cross-repository lightweight reviews skip repository context because they do not run the full local-tree workflow. + +Code-review agents receive the generic context headed by its label. The build-and-test role receives recommended tests, required configurations, and verification notes. Required roles are merged into the normal roster without duplication, and only when the review's effort, topology, and mode already permit them — a manifest cannot inflate a medium review with the adversarial personas, re-add whole-diff walkers to a chunked fan-out, or demand a tree-grepping role from a review with no tree. Composition discloses unverified dimensions as non-blocking proof boundaries; a present-but-invalid context fails every consumer closed rather than being silently dropped anywhere. + +Status: this is the foundation — the contract, the command, and the downstream consumers, exercised by unit tests and the review skill. No `.qwen/review-context.json` ships with this change, so nothing beyond tests runs end to end until a repository adopts one. diff --git a/docs/users/features/code-review.md b/docs/users/features/code-review.md index 026af0d5fcb..63e16219904 100644 --- a/docs/users/features/code-review.md +++ b/docs/users/features/code-review.md @@ -266,6 +266,33 @@ You can customize review criteria per project. `/review` reads rules from these Rules are injected into the LLM review agents (0-6) as additional criteria. For PR reviews, rules are read from the **base branch** to prevent a malicious PR from injecting bypass rules. +## Repository Context + +Repositories can hand the reviewers bounded, repository-specific guidance by committing a strict JSON manifest to `.qwen/review-context.json`. At medium or high effort, `/review` reads the manifest after capturing the plan and attaches the matching guidance before any agent launches: + +```json +{ + "version": 1, + "label": "Example repository", + "rules": [ + { + "paths": ["packages/*/src/**"], + "domains": ["runtime"], + "relatedPaths": ["packages/runtime/src/**"], + "recommendedTests": ["npm run test:runtime"], + "requiredConfigurations": ["debug"], + "requiredAgents": ["test-matrix"], + "unverifiedDimensions": ["Alternate runtime was not exercised"], + "verificationNotes": ["Use the repository native test runner"] + } + ] +} +``` + +A rule applies when any changed file matches one of its `paths` globs (`*`, `?`, and `**` segments; case-sensitive). All matching rules merge their guidance: domains and related files for the review agents, recommended tests and required configurations for the build-and-test agent, extra reviewer roles (honoured only when the chosen effort and topology run them), and proof boundaries the final review discloses as unverified dimensions. Arrays may be written in any order; duplicate entries are rejected. + +For PR reviews the manifest is read from the merge base, so the PR under review cannot opt itself into or out of guidance; local reviews read it from the current worktree. Low-effort and cross-repository reviews skip repository context. The full contract and trust model live in the [design doc](../../design/review-repository-context.md). + ## Issue Fidelity For bugfix PRs, the Issue Fidelity agent fetches issue evidence directly instead of relying on PR description text. It uses `gh pr view --repo --json closingIssuesReferences` for GitHub's strong closing-issue metadata, then `gh issue view --repo / --json title,body,comments` for the original report and discussion — the `--json` form includes the issue **body** (the reporter's original repro), which `--comments` alone omits, and the issue's own repository is read from each reference (a PR can close an issue in a different repo). This agent runs only for PR targets; local-diff and file-path reviews skip it. diff --git a/integration-tests/cli/qwen-serve-streaming.test.ts b/integration-tests/cli/qwen-serve-streaming.test.ts index a9ab2b2ce8f..4592b0e6f59 100644 --- a/integration-tests/cli/qwen-serve-streaming.test.ts +++ b/integration-tests/cli/qwen-serve-streaming.test.ts @@ -49,7 +49,6 @@ const CLI_BIN = process.env['TEST_CLI_PATH'] ?? path.resolve(__dirname, '../../packages/cli/dist/index.js'); const TOKEN = 'streaming-integ-secret'; -const REPO_ROOT = path.resolve(__dirname, '../..'); // Windows: this suite shells out to `pgrep` / `kill -KILL` to simulate // child-process crashes for the SIGKILL → `session_died` test, and those @@ -78,6 +77,7 @@ let base = ''; let client: DaemonClient; let fakeServer: FakeOpenAIServer; let homeDir = ''; +let workspaceDir = ''; let pendingWritePath = ''; beforeAll(async () => { @@ -131,6 +131,7 @@ beforeAll(async () => { ui: { enableFollowupSuggestions: false }, }), ); + workspaceDir = mkdtempSync(path.join(tmpdir(), 'qwen-serve-streaming-ws-')); daemon = spawn( process.execPath, [ @@ -143,16 +144,19 @@ beforeAll(async () => { '--hostname', '127.0.0.1', // Per #3803 §02 (1 daemon = 1 workspace), pin the bound - // workspace so every `createOrAttachSession({ workspaceCwd: - // REPO_ROOT })` below matches. Without this the daemon inherits - // the test runner's cwd (CI / IDE-launcher / direct vitest - // invocations all differ) and every session create returns - // 400 workspace_mismatch — the SSE / permission / Last-Event-ID - // tests below would all silently 404. Same fix the sibling routes test - // received earlier in this PR — missed in this file in the original §02 - // pass. + // workspace so every `createOrAttachSession({ workspaceCwd })` + // below matches. Without this the daemon inherits the test + // runner's cwd (CI / IDE-launcher / direct vitest invocations + // all differ) and every session create returns 400 + // workspace_mismatch — the SSE / permission / Last-Event-ID + // tests below would all silently 404. A scratch workspace (not + // the checkout) also keeps sessions hermetic: the daemon merges + // the workspace's `.qwen/settings.json` into every session, and + // a stray one on a shared runner (e.g. a `tools.sandbox` mode or + // a `tools.core` allowlist missing `todo_write`) silently breaks + // the Stop Guard flow below. '--workspace', - REPO_ROOT, + workspaceDir, ], { stdio: ['ignore', 'pipe', 'pipe'], @@ -211,6 +215,9 @@ afterAll(async () => { if (homeDir) { rmSync(homeDir, { recursive: true, force: true }); } + if (workspaceDir) { + rmSync(workspaceDir, { recursive: true, force: true }); + } }, 15_000); /** Open an authenticated SSE stream and yield parsed frames. */ @@ -241,7 +248,7 @@ async function* sseFrames( describePOSIX('qwen serve — child-crash recovery (real SIGKILL)', () => { it('publishes session_died after the qwen --acp child is SIGKILL-ed', async () => { const session = await client.createOrAttachSession({ - workspaceCwd: REPO_ROOT, + workspaceCwd: workspaceDir, }); // Find the daemon's direct `--acp` child PID. @@ -295,7 +302,7 @@ describePOSIX('qwen serve — child-crash recovery (real SIGKILL)', () => { ); // Listing must NOT show the dead session. - const remaining = await client.listWorkspaceSessions(REPO_ROOT); + const remaining = await client.listWorkspaceSessions(workspaceDir); // Explicit `s` type for resilience against a stale dist .d.ts // in the reviewer's tsc env (see same note in routes.test.ts). expect( @@ -306,7 +313,7 @@ describePOSIX('qwen serve — child-crash recovery (real SIGKILL)', () => { // Retry must spawn fresh, not reuse the corpse. const fresh = await client.createOrAttachSession({ - workspaceCwd: REPO_ROOT, + workspaceCwd: workspaceDir, }); expect(fresh.sessionId).not.toBe(session.sessionId); expect(fresh.attached).toBe(false); @@ -316,7 +323,7 @@ describePOSIX('qwen serve — child-crash recovery (real SIGKILL)', () => { describePOSIX('qwen serve — multi-client first-responder permission', () => { it('fans out permission_request to both subscribers; only one vote wins', async () => { const session = await client.createOrAttachSession({ - workspaceCwd: REPO_ROOT, + workspaceCwd: workspaceDir, }); // Pin the session to `default` approval mode. The ACP child @@ -448,7 +455,7 @@ describePOSIX('qwen serve — multi-client first-responder permission', () => { describePOSIX('qwen serve — Last-Event-ID resume', () => { it('reconnect with Last-Event-ID:N yields events with id > N', async () => { const session = await client.createOrAttachSession({ - workspaceCwd: REPO_ROOT, + workspaceCwd: workspaceDir, }); // Fire a short prompt to populate the bus. @@ -494,7 +501,7 @@ describePOSIX('qwen serve — Last-Event-ID resume', () => { describePOSIX('qwen serve — daemon Todo Stop Guard replay', () => { it('continues after prompt admission without an SSE client and replays the bounded attempts', async () => { const session = await client.createOrAttachSession({ - workspaceCwd: REPO_ROOT, + workspaceCwd: workspaceDir, }); const requestStart = fakeServer.requests.length; const guardMarker = `todo-guard-e2e-${requestStart}`; diff --git a/packages/cli/src/commands/review.test.ts b/packages/cli/src/commands/review.test.ts index f7129ac177a..40a2ecc4cd1 100644 --- a/packages/cli/src/commands/review.test.ts +++ b/packages/cli/src/commands/review.test.ts @@ -44,6 +44,7 @@ describe('reviewCommand', () => { 'fetch-pr', 'capture-local', 'plan-diff', + 'repo-context', 'pr-context', 'comment-status', 'load-rules', diff --git a/packages/cli/src/commands/review.ts b/packages/cli/src/commands/review.ts index 00331ec2393..7099d729ad1 100644 --- a/packages/cli/src/commands/review.ts +++ b/packages/cli/src/commands/review.ts @@ -15,6 +15,7 @@ import { findingsCommand } from './review/findings.js'; import { fetchPrCommand } from './review/fetch-pr.js'; import { captureLocalCommand } from './review/capture-local.js'; import { planDiffCommand } from './review/plan-diff.js'; +import { repoContextCommand } from './review/repo-context.js'; import { prContextCommand } from './review/pr-context.js'; import { commentStatusCommand } from './review/comment-status.js'; import { loadRulesCommand } from './review/load-rules.js'; @@ -49,6 +50,7 @@ export const reviewCommand: CommandModule = { .command(fetchPrCommand) .command(captureLocalCommand) .command(planDiffCommand) + .command(repoContextCommand) .command(prContextCommand) .command(commentStatusCommand) .command(loadRulesCommand) @@ -74,7 +76,7 @@ export const reviewCommand: CommandModule = { .command(cleanupCommand) .demandCommand( 1, - 'Specify a subcommand: run, parse-args, fetch-pr, capture-local, plan-diff, pr-context, comment-status, load-rules, agent-prompt, build-test, base-tree, test-delta, drive, mock-provider, extract-step, script-lint, resolve-anchors, check-coverage, cost-ledger, presubmit, test-efficacy, test-plan, findings, publish-assets, compose-review, save-artifact, submit, or cleanup.', + 'Specify a subcommand: run, parse-args, fetch-pr, capture-local, plan-diff, repo-context, pr-context, comment-status, load-rules, agent-prompt, build-test, base-tree, test-delta, drive, mock-provider, extract-step, script-lint, resolve-anchors, check-coverage, cost-ledger, presubmit, test-efficacy, test-plan, findings, publish-assets, compose-review, save-artifact, submit, or cleanup.', ) .version(false), handler: () => { diff --git a/packages/cli/src/commands/review/agent-prompt.test.ts b/packages/cli/src/commands/review/agent-prompt.test.ts index 33285c42e94..12f8362ab90 100644 --- a/packages/cli/src/commands/review/agent-prompt.test.ts +++ b/packages/cli/src/commands/review/agent-prompt.test.ts @@ -1830,6 +1830,103 @@ describe('buildRoleBrief — every agent, not just the territory ones', () => { expect(p).not.toMatch(/If you find no issues, say/i); }); + it('injects generic repository context into reviewers and a narrow verification boundary into Agent 7', () => { + const contextPlan = { + ...PR_PLAN, + repositoryContext: { + version: 1, + provider: 'fake-provider', + label: 'Example project', + domains: ['compiler', 'runtime'], + relatedPaths: ['src/compiler.ts', 'src/runtime.ts'], + recommendedTests: ['test:compiler'], + requiredConfigurations: ['debug', 'linux-x64'], + requiredAgents: ['test-matrix'], + unverifiedDimensions: ['Alternate runtime was not exercised'], + verificationNotes: ['Use the repository native test runner'], + }, + }; + + // Negative pins: roles outside the code-reviewing set and outside the + // manifest's required agents get nothing. A `brief.reviewsCode ||` → + // `true ||` regression would hand Agent 0 (issue fidelity, not code + // review) the full block on every context-bearing plan, and would give + // it to a role the manifest did not require, with the suite green. + expect(buildRoleBrief(contextPlan, '0')).not.toContain( + 'Example project repository context', + ); + expect( + buildRoleBrief( + { + ...contextPlan, + repositoryContext: { + ...contextPlan.repositoryContext, + requiredAgents: [], + }, + }, + 'test-matrix', + ), + ).not.toContain('Example project repository context'); + + const reviewerBrief = buildRoleBrief(contextPlan, '1a'); + expect(reviewerBrief).toContain('Example project repository context'); + expect(reviewerBrief).toContain('compiler, runtime'); + expect(reviewerBrief).toContain('src/compiler.ts'); + expect(reviewerBrief).toContain('test:compiler'); + expect(reviewerBrief).toContain('debug, linux-x64'); + expect(reviewerBrief).toContain('Alternate runtime was not exercised'); + expect(reviewerBrief).toContain('Use the repository native test runner'); + // Section adjacency: each field is pinned under ITS OWN label, or a + // rendering swap between two same-shaped arrays ships green while + // reviewers are told the repository's proof boundaries are its + // verification instructions — and vice versa. + expect(reviewerBrief).toContain( + 'Related paths:\n- src/compiler.ts\n- src/runtime.ts', + ); + expect(reviewerBrief).toContain( + 'Unverified dimensions:\n- Alternate runtime was not exercised', + ); + expect(reviewerBrief).toContain( + 'Verification notes:\n- Use the repository native test runner', + ); + + const territoryBrief = buildChunkAgentPrompt(contextPlan, 13); + expect(territoryBrief).toContain('Example project repository context'); + expect(territoryBrief).toContain('src/compiler.ts'); + + const requiredAgentBrief = buildRoleBrief(contextPlan, 'test-matrix'); + expect(requiredAgentBrief).toContain('Example project repository context'); + expect(requiredAgentBrief).toContain('src/compiler.ts'); + expect(requiredAgentBrief).toContain('test:compiler'); + + // Positive pins for code-reviewing roles OUTSIDE the manifest + // allow-list that reach the block solely through `brief.reviewsCode`: + // a narrowing mutant that keeps every pinned role strips exactly these + // and ships green. + for (const role of ['verify', 'reverse-audit'] as const) { + expect(buildRoleBrief(contextPlan, role)).toContain( + 'Example project repository context', + ); + } + + const buildBrief = buildRoleBrief(contextPlan, '7'); + expect(buildBrief).not.toContain('Example project repository context'); + expect(buildBrief).not.toContain('compiler, runtime'); + expect(buildBrief).not.toContain('src/compiler.ts'); + expect(buildBrief).not.toContain('Alternate runtime was not exercised'); + expect(buildBrief).toContain('Repository-specific verification boundary'); + expect(buildBrief).toContain('test:compiler'); + expect(buildBrief).toContain('debug, linux-x64'); + expect(buildBrief).toContain('Use the repository native test runner'); + + // The --whole-diff path builds Agent 8's briefs; it carries the same + // block, or the one finder launched for a dominant domain is the one + // reviewer denied that domain's guidance. + const wholeDiff = buildWholeDiffBlock(contextPlan); + expect(wholeDiff).toContain('Example project repository context'); + expect(wholeDiff).toContain('src/compiler.ts'); + }); + it('carries the mutation-testing lens into Agent 5, equivalent-mutant escape hatch included', () => { // The all-role test above proves every brief gets the diff and the format; it // cannot see whether a *specific* lens reached its role. If prompt assembly diff --git a/packages/cli/src/commands/review/agent-prompt.ts b/packages/cli/src/commands/review/agent-prompt.ts index 81780932119..77abe7734b0 100644 --- a/packages/cli/src/commands/review/agent-prompt.ts +++ b/packages/cli/src/commands/review/agent-prompt.ts @@ -61,7 +61,15 @@ import { scheduleReverseAuditRound, type RoundSchedule, } from './lib/retirement.js'; -import { BRIEFS, type RoleId } from './lib/agent-briefs.js'; +import { + BRIEFS, + isRepositoryContextRoleId, + type RoleId, +} from './lib/agent-briefs.js'; +import { + repositoryContextOf, + type RepositoryContext, +} from './lib/repository-context.js'; import { pathRulesFor } from './lib/path-rules.js'; import { requiredAgents, @@ -113,6 +121,7 @@ interface PlanReport { ownerRepo?: unknown; worktreePath?: unknown; mergeBaseSha?: unknown; + repositoryContext?: unknown; } /** A heavy file's entry, which is the only kind an invariant agent can be built from. */ @@ -366,6 +375,11 @@ export function buildChunkAgentPrompt( parts.push('', '## Project rules', '', rules.trim()); } + const repositoryContext = repositoryContextOf(report); + if (repositoryContext) { + parts.push('', ...repositoryContextBlock(repositoryContext)); + } + // Deliberately NOT included: a sentence for the agent to recite when it finds // nothing. Every real launch handed the agent its own receipt text — `If you // find no issues, say "No issues found — reviewed chunk 13 (...)"` — and an @@ -483,7 +497,13 @@ export function buildWholeDiffBlock( rules?: string, ): string { const diffPath = requireDiffPath(report); - return [...diffReadingBlock(report, diffPath), ...tail(rules)].join('\n'); + const parts = [...diffReadingBlock(report, diffPath)]; + const repositoryContext = repositoryContextOf(report); + if (repositoryContext) { + parts.push('', ...repositoryContextBlock(repositoryContext)); + } + parts.push(...tail(rules)); + return parts.join('\n'); } /** The diff path, or the error this whole command exists to make impossible. */ @@ -724,6 +744,42 @@ function invariantFileBlock( return parts; } +function contextList(values: string[]): string[] { + return values.length > 0 ? values.map((value) => `- ${value}`) : ['- (none)']; +} + +function repositoryContextBlock(context: RepositoryContext): string[] { + return [ + `## ${context.label} repository context`, + '', + `Domains: ${context.domains.join(', ') || '(none)'}`, + '', + 'Related paths:', + ...contextList(context.relatedPaths), + '', + `Recommended tests: ${context.recommendedTests.join(', ') || '(none)'}`, + `Required configurations: ${context.requiredConfigurations.join(', ') || '(none)'}`, + '', + 'Unverified dimensions:', + ...contextList(context.unverifiedDimensions), + '', + 'Verification notes:', + ...contextList(context.verificationNotes), + ]; +} + +function repositoryBuildBoundary(context: RepositoryContext): string[] { + return [ + '## Repository-specific verification boundary', + '', + `Recommended tests: ${context.recommendedTests.join(', ') || '(none)'}`, + `Required configurations: ${context.requiredConfigurations.join(', ') || '(none)'}`, + '', + 'Verification notes:', + ...contextList(context.verificationNotes), + ]; +} + /** * The launch prompt for any role that is not a territory agent. * @@ -769,6 +825,20 @@ export function buildRoleBrief( } parts.push('## Your dimension', '', brief.brief); + const repositoryContext = repositoryContextOf(report); + if (role === '7') { + if (repositoryContext) { + parts.push('', ...repositoryBuildBoundary(repositoryContext)); + } + } else if ( + brief.reviewsCode || + (isRepositoryContextRoleId(role) && + repositoryContext?.requiredAgents.includes(role)) + ) { + if (repositoryContext) { + parts.push('', ...repositoryContextBlock(repositoryContext)); + } + } // Cross-repo lightweight mode: there is no tree, only the diff. Two briefs assume // one, and the degradation used to be a sentence the orchestrator was told to add diff --git a/packages/cli/src/commands/review/compose-review.test.ts b/packages/cli/src/commands/review/compose-review.test.ts index 9dd2923534e..1402ddc6dd4 100644 --- a/packages/cli/src/commands/review/compose-review.test.ts +++ b/packages/cli/src/commands/review/compose-review.test.ts @@ -24,6 +24,7 @@ import { countInlineFindings } from './lib/inline-counts.js'; import { composeReview, buildLedger, + repositoryContextGate, scriptLintGate, testPlanGate, composeReviewCommand, @@ -98,6 +99,7 @@ function plan( effort?: 'low' | 'medium' | 'high'; /** Override the fixture's 5000 — the low-signal floor reads this. */ srcDiffLines?: number; + repositoryContext?: unknown; } = {}, ): string { const p = join(dir, 'plan.json'); @@ -111,6 +113,9 @@ function plan( // The effort the capturing command recorded — the roster and the // reverse-audit floor both read it from here. ...(opts.effort ? { effort: opts.effort } : {}), + ...(opts.repositoryContext === undefined + ? {} + : { repositoryContext: opts.repositoryContext }), srcDiffLines: opts.srcDiffLines ?? 5000, diffLines: 5000, files: [{ path: 'a.ts', kind: 'source', removedLines: 0, heavy: false }], @@ -329,6 +334,7 @@ function coveredPlan( han?: boolean; effort?: 'low' | 'medium' | 'high'; srcDiffLines?: number; + repositoryContext?: unknown; } = {}, ): string { transcript('a1', goodPrompt(1), { toolCalls: 3 }); @@ -458,6 +464,197 @@ describe('composeReview — the low-signal Approve disclosure', () => { }); }); +describe('repository context proof boundary', () => { + it('derives unreviewed dimensions from the validated plan, not model input', () => { + const planPath = join(dir, 'repository-plan.json'); + writeFileSync( + planPath, + JSON.stringify({ + repositoryContext: { + version: 1, + provider: 'fake-provider', + label: 'Example project', + domains: ['runtime'], + relatedPaths: [], + recommendedTests: [], + requiredConfigurations: ['linux-x64'], + requiredAgents: ['test-matrix'], + unverifiedDimensions: ['Alternate runtime was not exercised'], + verificationNotes: [], + }, + }), + ); + expect(repositoryContextGate(planPath)).toEqual([ + '`Alternate runtime was not exercised` — the repository context marks this proof boundary as unverified', + ]); + }); + + it('renders manifest-controlled proof boundaries as inert Markdown', () => { + const planPath = join(dir, 'mention-plan.json'); + writeFileSync( + planPath, + JSON.stringify({ + repositoryContext: { + version: 1, + provider: 'manifest', + label: 'Example project', + domains: [], + relatedPaths: [], + recommendedTests: [], + requiredConfigurations: [], + requiredAgents: [], + unverifiedDimensions: ['@security-team'], + verificationNotes: [], + }, + }), + ); + expect(repositoryContextGate(planPath)).toEqual([ + '`@security-team` — the repository context marks this proof boundary as unverified', + ]); + }); + + it('caps the unverified-dimension disclosure at five entries', () => { + // The schema admits 128 dimensions x 512 chars; joined into one + // disclosure that outruns the review body's own size budget — the same + // cap discipline testPlanGate applies to its notes. + const planPath = join(dir, 'capped-plan.json'); + writeFileSync( + planPath, + JSON.stringify({ + repositoryContext: { + version: 1, + provider: 'fake-provider', + label: 'Example project', + domains: [], + relatedPaths: [], + recommendedTests: [], + requiredConfigurations: [], + requiredAgents: [], + unverifiedDimensions: Array.from( + { length: 8 }, + (_, index) => `dimension ${index}`, + ), + verificationNotes: [], + }, + }), + ); + expect(repositoryContextGate(planPath)).toEqual([ + ...Array.from( + { length: 5 }, + (_, index) => + `\`dimension ${index}\` — the repository context marks this proof boundary as unverified`, + ), + 'and 3 more', + ]); + }); + + it('returns no extra disclosure when the plan has no repository context', () => { + const planPath = join(dir, 'generic-plan.json'); + writeFileSync(planPath, JSON.stringify({ files: [] })); + expect(repositoryContextGate(planPath)).toEqual([]); + }); + + it('returns nothing for an unreadable plan but fails closed on a malformed context', () => { + // Unreadable plan: the coverage gate owns plan validity; the disclosure + // has nothing to say. Present-but-INVALID context: every consumer of the + // field fails closed, so the gate throws instead of silently dropping the + // disclosure. + const missing = join(dir, 'missing-plan.json'); + expect(repositoryContextGate(missing)).toEqual([]); + + const malformed = join(dir, 'malformed-plan.json'); + writeFileSync( + malformed, + JSON.stringify({ repositoryContext: { version: 1 } }), + ); + expect(() => repositoryContextGate(malformed)).toThrow( + 'unknown or missing fields', + ); + }); + + it('keeps the disclosure on a REQUEST_CHANGES body', () => { + // The RC render site is a separate code path from APPROVE; deleting the + // block there must fail the suite, not ship green. + const planPath = coveredPlan(undefined, { + repositoryContext: { + version: 1, + provider: 'fake-provider', + label: 'Example project', + domains: [], + relatedPaths: [], + recommendedTests: [], + requiredConfigurations: [], + requiredAgents: [], + unverifiedDimensions: ['Alternate runtime was not exercised'], + verificationNotes: [], + }, + }); + const result = composeReview({ + planPath, + env: ENV, + modelId: MODEL, + bodyCriticals: ['whole-PR blocker X'], + }); + expect(result.event).toBe('REQUEST_CHANGES'); + expect(result.body).toContain('Repository proof boundary (not a blocker)'); + expect(result.body).toContain('Alternate runtime was not exercised'); + }); + + it('keeps the disclosure when a cap downgrades the verdict to COMMENT', () => { + // An APPROVE capped at COMMENT renders through the COMMENT clause + // composer — the third render site — and the disclosure must survive + // exactly the verdicts where the reader most needs the boundary. + const planPath = coveredPlan(undefined, { + repositoryContext: { + version: 1, + provider: 'fake-provider', + label: 'Example project', + domains: [], + relatedPaths: [], + recommendedTests: [], + requiredConfigurations: [], + requiredAgents: [], + unverifiedDimensions: ['Alternate runtime was not exercised'], + verificationNotes: [], + }, + }); + const result = composeReview({ + planPath, + env: ENV, + modelId: MODEL, + cannotTellCriticals: ['SKILL.md:35 — full text unfetchable'], + }); + expect(result.event).toBe('COMMENT'); + expect(result.cappedBy).toContain('cannot-tell-existing-critical'); + expect(result.body).toContain('Repository proof boundary (not a blocker)'); + expect(result.body).toContain('Alternate runtime was not exercised'); + }); + + it('discloses repository proof boundaries without permanently capping approval', () => { + const planPath = coveredPlan(undefined, { + repositoryContext: { + version: 1, + provider: 'fake-provider', + label: 'Example project', + domains: ['runtime'], + relatedPaths: [], + recommendedTests: [], + requiredConfigurations: ['linux-x64'], + requiredAgents: [], + unverifiedDimensions: ['Alternate runtime was not exercised'], + verificationNotes: [], + }, + }); + + const result = composeReview({ planPath, env: ENV, modelId: MODEL }); + + expect(result.event).toBe('APPROVE'); + expect(result.cappedBy).not.toContain('unreviewed-dimension'); + expect(result.body).toContain('Repository proof boundary (not a blocker)'); + expect(result.body).toContain('Alternate runtime was not exercised'); + }); +}); + describe('composeReview — event caps (round-7 Critical #2: caps must reach every path)', () => { it('a cannot-tell existing Critical caps APPROVE at COMMENT and is serialized (round-7: body said Unresolved while event said APPROVE)', () => { const r = composeReview( diff --git a/packages/cli/src/commands/review/compose-review.ts b/packages/cli/src/commands/review/compose-review.ts index 4f6703368f7..3cfe58f99a6 100644 --- a/packages/cli/src/commands/review/compose-review.ts +++ b/packages/cli/src/commands/review/compose-review.ts @@ -44,6 +44,7 @@ import { reviewMode, type RosterPlan, } from './lib/roster.js'; +import { repositoryContextOf } from './lib/repository-context.js'; import { diffHashOf, type ScriptLintReport } from './script-lint.js'; import type { TestPlanReport } from './test-plan.js'; import { @@ -482,12 +483,18 @@ function composeReviewBody( // Test Plan rulings. Disclosed on every verdict and counted toward nothing — // see `testPlanGate` for why this one neither blocks nor caps. const testPlanNotes: string[] = []; + // Repository proof boundaries are also disclosures, not findings or permanent + // approval caps. The first schema has no validated evidence channel that could + // resolve one after a specialist inspects it, so capping here would make every + // affected review impossible to approve. + const repositoryContextNotes: string[] = []; if (input.planPath) { const gate = scriptLintGate(input.planPath); bodyCriticals.push(...gate.criticals); // render + count toward `c`, deterministic unreviewed.push(...gate.unreviewed); gateDisclosed.push(...gate.disclosed); testPlanNotes.push(...testPlanGate(input.planPath).notes); + repositoryContextNotes.push(...repositoryContextGate(input.planPath)); } // The Criticals a verifier must have ruled on before this review may post them as @@ -906,17 +913,21 @@ function composeReviewBody( // keeps its bare Approve — there, finding nothing is the expected outcome. let lowSignal: ComposeReviewResult['lowSignal'] = null; if (event === 'APPROVE' && input.planPath) { + let plan: RosterPlan | undefined; try { - const plan = JSON.parse( - readFileSync(input.planPath, 'utf8'), - ) as RosterPlan; + plan = JSON.parse(readFileSync(input.planPath, 'utf8')) as RosterPlan; + } catch { + // Unreadable plan, no disclosure — the coverage gate owns plan validity. + } + // A malformed repositoryContext inside an otherwise-readable plan is NOT + // swallowed here: requiredAgents throws, fail-closed like every other + // consumer of the field. On a real APPROVE the coverage gate already + // validated it. + if (plan) { const src = Number(plan.srcDiffLines ?? 0); if (src > LOW_SIGNAL_SRC_DIFF_LINES) { lowSignal = { agents: requiredAgents(plan).length, srcDiffLines: src }; } - } catch { - // Unreachable on a real APPROVE — the coverage gate already read this - // plan — and a disclosure must never take the review down. } } @@ -1199,6 +1210,15 @@ function composeReviewBody( }, ]; + const repositoryContextBlock: Bi[] = repositoryContextNotes.length + ? [ + { + en: `Repository proof boundary (not a blocker): ${repositoryContextNotes.join('; ')}.`, + zh: `仓库验证边界（非阻断）：${repositoryContextNotes.join('; ')}。`, + }, + ] + : []; + if (event === 'REQUEST_CHANGES') { // Empty body, except the disclosures: every clause whose state holds // appears on every event — a confirmed blocker must not squeeze out the @@ -1211,6 +1231,7 @@ function composeReviewBody( ...unverifiedTagsBlock, ...deferredBlock, ...testPlanBlock, + ...repositoryContextBlock, ...bodyCriticalBlock, ]; return { @@ -1233,8 +1254,13 @@ function composeReviewBody( { en: 'No issues found. LGTM! ✅', zh: '未发现问题。LGTM！✅' }, ...deferredBlock, ...testPlanBlock, + ...repositoryContextBlock, ], - deferredBlock.length || testPlanBlock.length ? '\n\n' : ' ', + deferredBlock.length || + testPlanBlock.length || + repositoryContextBlock.length + ? '\n\n' + : ' ', ), baseEvent, cappedBy, @@ -1357,6 +1383,10 @@ function composeReviewBody( // the reviewed tree does not bear out. clauses.push(...testPlanBlock); + // 6d. Repository proof boundaries (non-capping) — dimensions the context + // planner recommends disclosing without claiming the code is defective. + clauses.push(...repositoryContextBlock); + // 7. Body Criticals — on a COMMENT that stands where a REQUEST_CHANGES // would have been: the presubmit carve-out, and the unverified-blockers // cap. Either way the body copy is the ONLY copy of an unanchorable @@ -1469,6 +1499,37 @@ const fetchPrBodyViaGh: PrBodyFetcher = (ownerRepo, prNumber) => { return (JSON.parse(json) as { body?: string }).body ?? ''; }; +export function repositoryContextGate(planPath: string): string[] { + let plan: RosterPlan; + try { + plan = JSON.parse(readFileSync(planPath, 'utf8')) as RosterPlan; + } catch { + // An unreadable plan has nothing to disclose; the coverage gate owns plan + // validity and already fails closed on it. + return []; + } + // A PRESENT-but-invalid context is a corrupted plan, and every consumer of + // this field fails closed on one — coverage throws, the roster throws; the + // disclosure cannot be the one place that silently shrugs. + const context = repositoryContextOf(plan); + const dimensions = context?.unverifiedDimensions ?? []; + // The same cap discipline testPlanGate applies: unbounded entries joined + // into one disclosure drown the verdict they ride on — and at the schema + // bounds (128 x 512 chars) the paragraph outruns the review body's own + // budget before any other content gets a word in. + const MAX_DIMENSIONS = 5; + const disclosed = dimensions + .slice(0, MAX_DIMENSIONS) + .map( + (dimension) => + `${mdField(dimension)} — the repository context marks this proof boundary as unverified`, + ); + if (dimensions.length > MAX_DIMENSIONS) { + disclosed.push(`and ${dimensions.length - MAX_DIMENSIONS} more`); + } + return disclosed; +} + /** * Read the script-lint report the orchestrator wrote and turn it into verdict * inputs, deterministically. Returns the pre-confirmed `[lint]` Criticals (a diff --git a/packages/cli/src/commands/review/lib/agent-briefs.ts b/packages/cli/src/commands/review/lib/agent-briefs.ts index aa2187721d9..67058d228b5 100644 --- a/packages/cli/src/commands/review/lib/agent-briefs.ts +++ b/packages/cli/src/commands/review/lib/agent-briefs.ts @@ -55,6 +55,36 @@ export type RoleId = | 'verify' | 'reverse-audit'; +/** + * The roles a repository context may require. One list is the single source for + * BOTH the type and the runtime guard: an allow-list the type admitted while the + * guard rejected it (or the reverse) would make the `is` predicate a lie, so + * neither half is written by hand any more. + */ +export const REPOSITORY_CONTEXT_ROLES = [ + '1a', + '1b', + '1c', + '2', + '3a', + '3b', + '3c', + '4', + '5', + '6a', + '6b', + '6c', + 'test-matrix', +] as const satisfies readonly RoleId[]; + +export type RepositoryContextRoleId = (typeof REPOSITORY_CONTEXT_ROLES)[number]; + +export function isRepositoryContextRoleId( + value: string, +): value is RepositoryContextRoleId { + return (REPOSITORY_CONTEXT_ROLES as readonly string[]).includes(value); +} + export interface Brief { /** How the role is named to a human reading a coverage failure. */ label: string; diff --git a/packages/cli/src/commands/review/lib/manifest-repository-context.test.ts b/packages/cli/src/commands/review/lib/manifest-repository-context.test.ts new file mode 100644 index 00000000000..dc4046c123c --- /dev/null +++ b/packages/cli/src/commands/review/lib/manifest-repository-context.test.ts @@ -0,0 +1,1012 @@ +/** + * @license + * Copyright 2026 Qwen Team + * SPDX-License-Identifier: Apache-2.0 + */ + +import { + chmodSync, + mkdirSync, + mkdtempSync, + realpathSync, + rmSync, + symlinkSync, + writeFileSync, +} from 'node:fs'; +import { tmpdir } from 'node:os'; +import { dirname, join } from 'node:path'; +import { afterAll, describe, expect, it } from 'vitest'; +import { + manifestRepositoryContextProvider, + MAX_GLOB_CANDIDATES, + MAX_MATCH_WORK, +} from './manifest-repository-context.js'; +import { MAX_IDENTITY_BYTES } from './repository-context.js'; + +const worktrees: string[] = []; + +function temp(): string { + const root = realpathSync(mkdtempSync(join(tmpdir(), 'manifest-context-'))); + worktrees.push(root); + return root; +} + +// Several fixtures hold 16k-entry trees; leaking them exhausts a tmpfs +// /tmp within a handful of runs. +afterAll(() => { + for (const root of worktrees) { + rmSync(root, { recursive: true, force: true }); + } +}); + +function write(path: string, content = ''): void { + mkdirSync(dirname(path), { recursive: true }); + writeFileSync(path, content); +} + +function manifest(overrides: object = {}): string { + return JSON.stringify({ + version: 1, + label: 'Example repository', + rules: [{ paths: ['src/**'] }], + ...overrides, + }); +} + +function provide( + worktree: string, + changedPaths: string[], + content: string | null, +) { + return manifestRepositoryContextProvider.provide({ + worktree, + changedPaths, + readIdentityFile: () => content, + }); +} + +describe('manifest repository context provider', () => { + it('matches rules, merges fields, and expands related files', () => { + const worktree = temp(); + write(join(worktree, 'src', 'change.ts')); + write(join(worktree, 'src', 'support.ts')); + write(join(worktree, 'src', '.hidden.ts')); + const content = manifest({ + rules: [ + { + paths: ['src/**'], + relatedPaths: ['src/**'], + domains: ['runtime'], + recommendedTests: ['test:fast'], + requiredConfigurations: ['debug'], + requiredAgents: ['test-matrix'], + unverifiedDimensions: ['Alternate configuration'], + verificationNotes: ['Run focused checks'], + }, + { + paths: ['src/*.ts'], + domains: ['compiler', 'runtime'], + recommendedTests: ['test:fast', 'test:full'], + }, + ], + }); + + expect(provide(worktree, ['src/change.ts'], content)).toEqual({ + version: 1, + provider: 'manifest', + label: 'Example repository', + domains: ['compiler', 'runtime'], + relatedPaths: ['src/.hidden.ts', 'src/support.ts'], + recommendedTests: ['test:fast', 'test:full'], + requiredConfigurations: ['debug'], + requiredAgents: ['test-matrix'], + unverifiedDimensions: ['Alternate configuration'], + verificationNotes: ['Run focused checks'], + }); + }); + + it('returns null without a manifest or matching rule', () => { + const worktree = temp(); + expect(provide(worktree, ['src/change.ts'], null)).toBeNull(); + expect( + provide( + worktree, + ['src/change.ts'], + manifest({ rules: [{ paths: ['docs/**'] }] }), + ), + ).toBeNull(); + }); + + it('attaches nothing for an empty change set', () => { + // `[].some(...)` is false, so no rule matches an empty diff — pinning the + // filter against a `.every` mutation, under which EVERY rule matches. + const worktree = temp(); + const content = manifest({ + rules: [{ paths: ['**'], requiredAgents: ['test-matrix'] }], + }); + expect(provide(worktree, [], content)).toBeNull(); + }); + + it.each([ + ['malformed JSON', '{'], + ['unsupported manifest version', manifest({ version: 2 })], + ['unknown top-level field', manifest({ extra: true })], + ['missing required field', JSON.stringify({ version: 1, rules: [] })], + [ + 'unknown rule field', + manifest({ rules: [{ paths: ['src/**'], extra: [] }] }), + ], + ['duplicate array', manifest({ rules: [{ paths: ['src/**', 'src/**'] }] })], + ['unsafe traversal glob', manifest({ rules: [{ paths: ['src/../**'] }] })], + ['unsafe absolute glob', manifest({ rules: [{ paths: ['/src/**'] }] })], + ['unsafe brace glob', manifest({ rules: [{ paths: ['src/{a,b}.ts'] }] })], + ['unsafe extglob', manifest({ rules: [{ paths: ['src/+(a).ts'] }] })], + [ + 'unbounded related glob', + manifest({ + rules: [{ paths: ['src/**'], relatedPaths: ['**/*.ts'] }], + }), + ], + [ + 'root-level wildcard related glob', + manifest({ + rules: [{ paths: ['src/**'], relatedPaths: ['*.ts'] }], + }), + ], + [ + 'first-segment wildcard related glob', + manifest({ + rules: [{ paths: ['src/**'], relatedPaths: ['src*/*.ts'] }], + }), + ], + ])('fails closed for %s', (_name, content) => { + expect(() => provide(temp(), ['src/change.ts'], content)).toThrow(); + }); + + it('fails closed when the rule count exceeds the parse bound', () => { + // MAX_RULES is the parse-time/memory bound protecting the step from + // adversarial manifests. Nothing else compensates: `paths: []` passes + // the array validator and contributes nothing to the total-`paths` cap, + // so an unbounded rule count would walk the full per-rule loop. + expect(() => + provide( + temp(), + ['src/change.ts'], + manifest({ rules: Array.from({ length: 129 }, () => ({ paths: [] })) }), + ), + ).toThrow('rules is invalid'); + }); + + it('fails closed when the total paths globs outgrow the matching bound', () => { + // The rule filter tests every changed path against every `paths` glob, + // so the total across rules — not each rule's array — is capped. + const worktree = temp(); + const rules = [ + { paths: Array.from({ length: 128 }, (_, index) => `area-${index}.ts`) }, + { paths: ['src/**'] }, + ]; + expect(() => + provide(worktree, ['src/change.ts'], manifest({ rules })), + ).toThrow('paths exceeds limit'); + }); + + it('fails closed when merged fields or glob lists outgrow the wire bound', () => { + const worktree = temp(); + // Every single rule honors the 128-item bound; the MERGE does not. + expect(() => + provide( + worktree, + ['src/change.ts'], + manifest({ + rules: [ + { + paths: ['src/**'], + domains: Array.from( + { length: 128 }, + (_, index) => `domain-a-${String(index).padStart(3, '0')}`, + ), + }, + { + paths: ['src/**'], + domains: Array.from( + { length: 128 }, + (_, index) => `domain-b-${String(index).padStart(3, '0')}`, + ), + }, + ], + }), + ), + ).toThrow('domains exceeds limit'); + expect(() => + provide( + worktree, + ['src/change.ts'], + manifest({ + rules: Array.from({ length: 65 }, (_, index) => ({ + paths: ['src/**'], + verificationNotes: [ + `note-a-${String(index).padStart(3, '0')}`, + `note-b-${String(index).padStart(3, '0')}`, + ], + })), + }), + ), + ).toThrow('verificationNotes exceeds limit'); + // The merged `relatedPaths` pattern list is capped BEFORE any scan, or a + // max-cardinality manifest stalls expansion for minutes. + expect( + () => + provide( + worktree, + ['src/change.ts'], + manifest({ + rules: [ + { + paths: ['src/**'], + relatedPaths: Array.from( + { length: 128 }, + (_, index) => `p-a/${index}.ts`, + ), + }, + { + paths: ['src/**'], + relatedPaths: Array.from( + { length: 128 }, + (_, index) => `p-b/${index}.ts`, + ), + }, + ], + }), + ), + // Distinct from the resolved-files cap in the expansion: the operator + // must be able to tell whether to trim the glob list or the globs. + ).toThrow('relatedPaths glob list exceeds limit'); + }); + + it('rejects globs that enter dependency or build-output trees', () => { + // The never-descend invariant is enforced for entries found during + // recursion; without rejecting these at validation, a pattern ROOTED + // below a skipped tree bypasses it through the scan roots (and can + // exhaust the entry ceiling mid-scan on an installed checkout). + const worktree = temp(); + const content = (paths: string[], relatedPaths: string[]) => + manifest({ rules: [{ paths, relatedPaths }] }); + expect(() => + provide( + worktree, + ['src/change.ts'], + content(['src/**'], ['coverage/**']), + ), + ).toThrow('relatedPaths enters a skipped directory'); + expect(() => + provide( + worktree, + ['src/change.ts'], + content(['src/**'], ['src/dist/**']), + ), + ).toThrow('relatedPaths enters a skipped directory'); + expect(() => + provide( + worktree, + ['src/change.ts'], + content(['node_modules/vendor/**/*.ts'], ['src/**']), + ), + ).toThrow('paths enters a skipped directory'); + }); + + it('excludes related file and directory symlink escapes', () => { + const root = temp(); + const worktree = join(root, 'worktree'); + const outside = join(root, 'outside'); + write(join(outside, 'secret.ts')); + write(join(worktree, 'src', 'safe.ts')); + symlinkSync(join(outside, 'secret.ts'), join(worktree, 'src', 'escape.ts')); + symlinkSync(outside, join(worktree, 'src', 'external')); + + const context = provide( + worktree, + ['src/change.ts'], + manifest({ + rules: [{ paths: ['src/**'], relatedPaths: ['src/**'] }], + }), + ); + expect(context?.relatedPaths).toEqual(['src/safe.ts']); + }); + + // A backslash is a path separator on Windows, so the POSIX-only filename + // shapes this guards against cannot exist there. + it.skipIf(process.platform === 'win32')( + 'skips related files with POSIX-legal unsafe name bytes', + () => { + // A backslash and a control character are legal filename bytes on + // POSIX; such files must be skipped rather than failing validation + // for the whole review. + const worktree = temp(); + write(join(worktree, 'src', 'safe.ts')); + write(join(worktree, 'src', 'foo\\bar.ts')); + write(join(worktree, 'src', 'foo\u0001bar.ts')); + const context = provide( + worktree, + ['src/change.ts'], + manifest({ + rules: [{ paths: ['src/**'], relatedPaths: ['src/**'] }], + }), + ); + expect(context?.relatedPaths).toEqual(['src/safe.ts']); + }, + ); + + it.each([ + '.git', + '.next', + '.turbo', + 'coverage', + 'dist', + 'node_modules', + 'out', + 'target', + ])( + 'never descends into %s', + (name) => { + // Without the skip this installed-shape tree exceeds the visited-entry + // ceiling mid-scan; with it, only source entries count. Every member of + // the skip set is pinned, or removing any single one ships green. + const worktree = temp(); + write(join(worktree, 'src', 'keep.ts')); + const skipped = join(worktree, 'src', name); + mkdirSync(skipped, { recursive: true }); + for (let index = 0; index < MAX_GLOB_CANDIDATES; index++) { + writeFileSync( + join(skipped, `${String(index).padStart(6, '0')}.js`), + '', + ); + } + expect( + provide( + worktree, + ['src/change.ts'], + manifest({ + rules: [{ paths: ['src/**'], relatedPaths: ['src/**'] }], + }), + )?.relatedPaths, + ).toEqual(['src/keep.ts']); + }, + 30_000, + ); + + it('accepts a scan sitting exactly at the resolved-file bound', () => { + // The reject side pins 129 matches; this accept pin sits exactly at + // 128, where a `>` → `>=` regression would fail a legal manifest + // closed at the source's own calibration point. 127 wildcard matches + // plus one static entry also exercise the cap check in BOTH branches. + const worktree = temp(); + const source = join(worktree, 'src'); + mkdirSync(source); + for (let index = 0; index < 127; index++) { + writeFileSync(join(source, `${String(index).padStart(3, '0')}.ts`), ''); + } + write(join(worktree, 'zz', 'extra.ts')); + expect( + provide( + worktree, + ['src/change.ts'], + manifest({ + rules: [ + { + paths: ['src/**'], + relatedPaths: ['src/**', 'zz/extra.ts'], + }, + ], + }), + )?.relatedPaths, + ).toHaveLength(128); + }); + + it('accepts a scan visiting exactly the visited-entry ceiling', () => { + // The reject side pins 16,385 entries; this accept pin visits exactly + // MAX_GLOB_CANDIDATES. Empty directories count as entries too; the + // scan root itself does not. + const worktree = temp(); + const source = join(worktree, 'src'); + mkdirSync(source); + for (let index = 0; index < MAX_GLOB_CANDIDATES - 1; index++) { + mkdirSync(join(source, `d-${String(index).padStart(5, '0')}`)); + } + writeFileSync(join(source, 'keep.ts'), ''); + expect( + provide( + worktree, + ['src/change.ts'], + manifest({ + rules: [{ paths: ['src/**'], relatedPaths: ['src/**'] }], + }), + )?.relatedPaths, + ).toEqual(['src/keep.ts']); + }, 60_000); + + it('fails closed as soon as related matches exceed the bound', () => { + const worktree = temp(); + const source = join(worktree, 'src'); + mkdirSync(source); + for (let index = 0; index < 129; index++) { + writeFileSync(join(source, `${String(index).padStart(3, '0')}.ts`), ''); + } + expect(() => + provide( + worktree, + ['src/change.ts'], + manifest({ + rules: [{ paths: ['src/**'], relatedPaths: ['src/**'] }], + }), + ), + ).toThrow('relatedPaths exceeds limit'); + }); + + it('fails closed on the static branch when merged matches exceed the bound', () => { + // 128 wildcard matches sit exactly at the bound, then a static root adds + // one more — the static-file branch enforces the same cap the directory + // branch does, or the wire validator reports a schema shape error instead. + const worktree = temp(); + const source = join(worktree, 'src'); + mkdirSync(source); + for (let index = 0; index < 128; index++) { + writeFileSync(join(source, `${String(index).padStart(3, '0')}.ts`), ''); + } + write(join(worktree, 'zz', 'extra.ts')); + expect(() => + provide( + worktree, + ['src/change.ts'], + manifest({ + rules: [ + { + paths: ['src/**'], + relatedPaths: ['src/**', 'zz/extra.ts'], + }, + ], + }), + ), + ).toThrow('relatedPaths exceeds limit'); + }); + + it('bounds candidate scanning even when matches are later excluded', () => { + const worktree = temp(); + const source = join(worktree, 'src'); + mkdirSync(source); + const changedPaths = Array.from( + { length: MAX_GLOB_CANDIDATES + 1 }, + (_, index) => { + const name = `${String(index).padStart(6, '0')}.ts`; + writeFileSync(join(source, name), ''); + return `src/${name}`; + }, + ); + expect(() => + provide( + worktree, + changedPaths, + manifest({ + rules: [{ paths: ['src/**'], relatedPaths: ['src/**'] }], + }), + ), + ).toThrow('scan exceeds limit'); + }, 30_000); + + it('expands nested related globs without double-counting the subsumed root', () => { + const worktree = temp(); + write(join(worktree, 'docs', 'a.ts')); + write(join(worktree, 'docs', 'api', 'b.ts')); + expect( + provide( + worktree, + ['src/change.ts'], + manifest({ + rules: [ + { + paths: ['src/**'], + relatedPaths: ['docs/**', 'docs/api/**'], + }, + ], + }), + )?.relatedPaths, + ).toEqual(['docs/a.ts', 'docs/api/b.ts']); + }); + + it('counts a subsumed subtree against the scan bound only once', () => { + // Double-scanning this tree against the shared counter visits ~2x8194 + // entries and fails a legal manifest closed at half the tree. + const worktree = temp(); + const api = join(worktree, 'docs', 'api'); + mkdirSync(api, { recursive: true }); + for (let index = 0; index < MAX_GLOB_CANDIDATES / 2; index++) { + mkdirSync(join(api, `dir-${String(index).padStart(5, '0')}`)); + } + write(join(worktree, 'docs', 'a.ts')); + expect( + provide( + worktree, + ['src/change.ts'], + manifest({ + rules: [ + { + paths: ['src/**'], + relatedPaths: ['docs/**', 'docs/api/**'], + }, + ], + }), + )?.relatedPaths, + ).toEqual(['docs/a.ts']); + }, 30_000); + + it('accepts unsorted manifest arrays and sorts the merged output', () => { + // The manifest is human-authored: only uniqueness is enforced there; the + // provider sorts before the wire format's strict sorted-and-unique + // validator sees the result. + const worktree = temp(); + write(join(worktree, 'src', 'a.ts')); + write(join(worktree, 'src', 'b.ts')); + const content = manifest({ + rules: [ + { + paths: ['src/b.ts', 'src/a.ts'], + relatedPaths: ['src/b.ts', 'src/a.ts'], + domains: ['zeta', 'alpha'], + }, + ], + }); + const context = provide(worktree, ['src/a.ts'], content); + expect(context?.domains).toEqual(['alpha', 'zeta']); + expect(context?.relatedPaths).toEqual(['src/b.ts']); + }); + + it('deduplicates related patterns before applying the merge bound', () => { + // 128 rules each contribute the same two patterns: 256 pre-dedup + // (OVER the cap) and 2 post-dedup (under it). A cap-before-dedup + // regression throws here; under it, two matching rules sharing one + // 100-pattern list would reject a legal, human-authored manifest. + const worktree = temp(); + for (let index = 0; index < 5; index++) { + write(join(worktree, 'src', `${index}.ts`)); + } + for (let index = 0; index < 4; index++) { + write(join(worktree, 'docs', `${index}.ts`)); + } + const rules = Array.from({ length: 128 }, () => ({ + paths: ['src/**'], + relatedPaths: ['src/**', 'docs/**'], + })); + expect( + provide(worktree, ['src/change.ts'], manifest({ rules }))?.relatedPaths, + ).toHaveLength(9); + }); + + it('fails closed when cumulative matching work exceeds the budget', () => { + // The visited-entry cap bounds a COUNT; the per-candidate matching + // work is a separate dimension (O(pattern segments x path segments) + // per memoised attempt). Deep literal chains keep each attempt cheap + // to run but maximal in charged segments, so the budget trips — with + // the scan-limit wording — long before the entry cap does, pinning + // the accounting without a minutes-long memo explosion. + const worktree = temp(); + let directory = join(worktree, 'x'); + for (let level = 0; level < 198; level++) { + directory = join(directory, 'a'); + } + directory = join(directory, 'z'); + mkdirSync(directory, { recursive: true }); + for (let index = 0; index < 60; index++) { + writeFileSync( + join(directory, `leaf-${String(index).padStart(4, '0')}.ts`), + '', + ); + } + const stem = `x/${'a/'.repeat(198)}*/`; + expect(() => + provide( + worktree, + ['x/change.ts'], + manifest({ + rules: [ + { + paths: ['x/**'], + relatedPaths: Array.from( + { length: 128 }, + (_, index) => `${stem}b${index}`, + ), + }, + ], + }), + ), + ).toThrow('matching work exceeds limit'); + }, 30_000); + + it('expands static related paths as their own files', () => { + const worktree = temp(); + write(join(worktree, 'src', 'main.ts')); + write(join(worktree, 'src', 'main.test.ts')); + expect( + provide( + worktree, + ['src/main.ts'], + manifest({ + rules: [{ paths: ['src/**'], relatedPaths: ['src/main.test.ts'] }], + }), + )?.relatedPaths, + ).toEqual(['src/main.test.ts']); + }); + + it('resolves a top-level static related entry as itself', () => { + // A completely static entry can never begin with a repository-wide + // wildcard, so the directory-prefix rule applies only to wildcard globs. + const worktree = temp(); + write(join(worktree, 'package.json'), '{}'); + expect( + provide( + worktree, + ['src/change.ts'], + manifest({ + rules: [{ paths: ['src/**'], relatedPaths: ['package.json'] }], + }), + )?.relatedPaths, + ).toEqual(['package.json']); + }); + + it('uses case-sensitive UTF-16 matching for rules and related expansion', () => { + const worktree = temp(); + write(join(worktree, 'src', 'X.TS')); + write(join(worktree, 'src', '😀.ts')); + expect( + provide( + worktree, + ['src/X.TS'], + manifest({ rules: [{ paths: ['src/*.ts'] }] }), + ), + ).toBeNull(); + expect( + provide( + worktree, + ['src/😀.ts'], + manifest({ rules: [{ paths: ['src/?.ts'] }] }), + ), + ).toBeNull(); + expect( + provide( + worktree, + ['src/😀.ts'], + manifest({ rules: [{ paths: ['src/??.ts'] }] }), + )?.label, + ).toBe('Example repository'); + }); + + it('matches multi-star segments in polynomial time', () => { + // `'ab*'` repeated in one segment is the catastrophic-backtracking shape + // the old compiled regex died on (a 45 s watchdog killed the probe at an + // 81-char filename); the matcher stays polynomial in pattern x value. + const worktree = temp(); + write(join(worktree, 'src', `${'ab'.repeat(40)}x`)); + write(join(worktree, 'src', 'ababab')); + const content = manifest({ + rules: [ + { + paths: ['src/**'], + relatedPaths: [`src/${'ab*'.repeat(30)}ab`, 'src/ab*ab*ab'], + }, + ], + }); + expect(provide(worktree, ['src/change.ts'], content)?.relatedPaths).toEqual( + ['src/ababab'], + ); + }, 10_000); + + it('produces deterministic code-unit sorted output', () => { + const worktree = temp(); + write(join(worktree, 'src', 'z.ts')); + write(join(worktree, 'src', 'A.ts')); + // A directory that is a strict prefix of a sibling file's name: the scan + // emits the directory's contents before the sibling ('eslint' sorts + // before 'eslint.config.js'), the REVERSE of code-unit order ('.' 0x2E + // < '/' 0x2F) — the shape the final sort exists to repair. + write(join(worktree, 'src', 'eslint', 'index.ts')); + write(join(worktree, 'src', 'eslint.config.js')); + const content = manifest({ + rules: [ + { + paths: ['src/**'], + relatedPaths: ['src/**'], + domains: ['zeta'], + }, + { + paths: ['src/*.ts'], + domains: ['Alpha'], + }, + ], + }); + const first = provide(worktree, ['src/change.ts'], content); + const second = provide(worktree, ['src/change.ts'], content); + expect(first).toEqual(second); + expect(first?.domains).toEqual(['Alpha', 'zeta']); + expect(first?.relatedPaths).toEqual([ + 'src/A.ts', + 'src/eslint.config.js', + 'src/eslint/index.ts', + 'src/z.ts', + ]); + }); + + it('fails closed before parsing an oversized manifest', () => { + // The size ceiling sits BEFORE JSON.parse: an attacker-committed + // manifest near the push size limits must not drive the parser's + // memory to the heap ceiling just to reach the bounded-array + // rejection. + expect(() => + provide(temp(), ['src/change.ts'], 'x'.repeat(MAX_IDENTITY_BYTES + 1)), + ).toThrow('exceeds the size limit'); + }); + + it('merges fields only from rules whose paths matched', () => { + // A rule scoped to another tree must not attach its fields to a review + // it does not match: `matched.flatMap` rewritten to + // `manifest.rules.flatMap` inflates every context with every rule and + // must turn red here. + const worktree = temp(); + write(join(worktree, 'docs', 'note.md')); + const content = manifest({ + rules: [ + { paths: ['docs/**'], domains: ['docs-domain'] }, + { + paths: ['tools/**'], + domains: ['tools-domain'], + requiredAgents: ['test-matrix'], + verificationNotes: ['tools note'], + }, + ], + }); + const context = provide(worktree, ['docs/note.md'], content); + expect(context?.domains).toEqual(['docs-domain']); + expect(context?.requiredAgents).toEqual([]); + expect(context?.verificationNotes).toEqual([]); + }); + + it('enforces the skip set case-insensitively', () => { + // A case-varied pattern or directory name walks into a skipped tree on + // every platform unless membership compares case-insensitively at both + // enforcement sites. + const worktree = temp(); + expect(() => + provide( + worktree, + ['src/change.ts'], + manifest({ rules: [{ paths: ['NODE_MODULES/**'] }] }), + ), + ).toThrow('paths enters a skipped directory'); + expect(() => + provide( + worktree, + ['src/change.ts'], + manifest({ + rules: [{ paths: ['src/**'], relatedPaths: ['src/DIST/**'] }], + }), + ), + ).toThrow('relatedPaths enters a skipped directory'); + write(join(worktree, 'src', 'keep.ts')); + write(join(worktree, 'src', 'NODE_MODULES', 'dep.js')); + expect( + provide( + worktree, + ['src/change.ts'], + manifest({ + rules: [{ paths: ['src/**'], relatedPaths: ['src/**'] }], + }), + )?.relatedPaths, + ).toEqual(['src/keep.ts']); + }); + + it('skips a static related entry that is itself a symlink', () => { + // The scan-root lstat guard needs its own pin: the recursion-level + // dirent check never sees a symlink that IS the scan root. + const worktree = temp(); + write(join(worktree, 'src', 'real.ts')); + symlinkSync('real.ts', join(worktree, 'src', 'link.ts')); + expect( + provide( + worktree, + ['src/change.ts'], + manifest({ + rules: [{ paths: ['src/**'], relatedPaths: ['src/link.ts'] }], + }), + )?.relatedPaths, + ).toEqual([]); + }); + + it.skipIf(process.platform === 'win32')( + 'excludes files reached through a symlinked interior scan-root component', + () => { + // The scan-root lstat guard inspects only the FINAL component; the + // containment check is the only defense against an escaping symlink + // MID-path. A head that swaps a base rule's directory for such a link + // would otherwise leak outside files into every reviewer prompt. + const root = temp(); + const worktree = join(root, 'worktree'); + const outside = join(root, 'outside'); + write(join(outside, 'v2', 'secret.ts')); + mkdirSync(join(worktree, 'docs'), { recursive: true }); + symlinkSync(outside, join(worktree, 'docs', 'api')); + expect( + provide( + worktree, + ['src/change.ts'], + manifest({ + rules: [ + { + paths: ['src/**'], + relatedPaths: ['docs/api/v2/**', 'docs/api/v2/secret.ts'], + }, + ], + }), + )?.relatedPaths, + ).toEqual([]); + }, + ); + + it('still scans names the skip set deliberately excludes', () => { + // The per-member tests pin the set in the SHRINK direction only; this + // pins that a name outside it stays scannable, or a perf-motivated + // addition rejects legal manifests with every suite green. + const worktree = temp(); + write(join(worktree, 'build', 'gen.ts')); + expect( + provide( + worktree, + ['src/change.ts'], + manifest({ + rules: [{ paths: ['src/**'], relatedPaths: ['build/**'] }], + }), + )?.relatedPaths, + ).toEqual(['build/gen.ts']); + }); + + it('dedupes identical scan roots before charging the scan bound', () => { + // Two patterns sharing one static prefix scan the shared root ONCE; a + // dedupe regression visits it per pattern and fails a legal manifest + // closed at half the tree. + const worktree = temp(); + const source = join(worktree, 'src'); + mkdirSync(source); + for (let index = 0; index < MAX_GLOB_CANDIDATES / 2 + 1; index++) { + mkdirSync(join(source, `d-${String(index).padStart(5, '0')}`)); + } + writeFileSync(join(source, 'keep.ts'), ''); + expect( + provide( + worktree, + ['src/change.ts'], + manifest({ + rules: [{ paths: ['src/**'], relatedPaths: ['src/**', 'src/*.ts'] }], + }), + )?.relatedPaths, + ).toEqual(['src/keep.ts']); + }, 30_000); + + it('keeps sibling scan roots when one string-prefixes the other', () => { + // The subsumption filter's trailing-`/` boundary: without it `src` + // string-prefix-matches `sr` and every related file under it is + // silently dropped. + const worktree = temp(); + write(join(worktree, 'sr', 'a.ts')); + write(join(worktree, 'src', 'b.ts')); + expect( + provide( + worktree, + ['src/change.ts'], + manifest({ + rules: [{ paths: ['src/**'], relatedPaths: ['sr/**', 'src/**'] }], + }), + )?.relatedPaths, + ).toEqual(['sr/a.ts', 'src/b.ts']); + }); + + it('fails closed when rule-filter matching work exceeds the budget', () => { + // The filter bills the same length-based budget the expansion does: a + // bulk change set crossed with schema-legal `paths` globs must fail + // closed in this sibling stage too instead of stalling the step. + const changed = Array.from( + { length: 1024 }, + (_, index) => + `src/${'c'.repeat(240)}${String(index).padStart(4, '0')}.ts`, + ); + const paths = Array.from( + { length: 128 }, + (_, index) => + `src/${'p'.repeat(240)}${String(index).padStart(3, '0')}/**`, + ); + expect(() => + provide(temp(), changed, manifest({ rules: [{ paths }] })), + ).toThrow('paths matching work exceeds limit'); + }, 30_000); + + it('accepts matching work sitting exactly at the budget', () => { + // The reject side pins an over-budget shape; this accept pin charges + // exactly MAX_MATCH_WORK (pattern length x path length per attempt), so + // a billing-multiplier or budget-halving regression fails a shape the + // calibration admits. Every file misses every pattern, so the whole + // budget is charged and nothing matches. + const worktree = temp(); + const source = join(worktree, 'src'); + mkdirSync(source); + const fileCount = 1024; + for (let index = 0; index < fileCount; index++) { + writeFileSync( + join(source, `${'f'.repeat(53)}${String(index).padStart(4, '0')}.ts`), + '', + ); + } + const patterns = Array.from( + { length: 128 }, + (_, index) => + `src/**/${'m'.repeat(118)}${String(index).padStart(3, '0')}`, + ); + const pathLength = `src/${'f'.repeat(53)}0000.ts`.length; + expect(fileCount * patterns.length * pathLength * patterns[0].length).toBe( + MAX_MATCH_WORK, + ); + expect( + provide( + worktree, + ['src/change.ts'], + manifest({ + rules: [{ paths: ['src/**'], relatedPaths: patterns }], + }), + )?.relatedPaths, + ).toEqual([]); + }, 30_000); + + it('expands related globs whose static prefix contains ?', () => { + // The prefix scanner must STOP at `?`: a break-condition regression + // makes the scan root the literal `src/a?c`, which does not exist, and + // the rule silently attaches nothing. + const worktree = temp(); + write(join(worktree, 'src', 'a1c', 'x.ts')); + write(join(worktree, 'src', 'a2c', 'y.ts')); + expect( + provide( + worktree, + ['src/change.ts'], + manifest({ + rules: [{ paths: ['src/**'], relatedPaths: ['src/a?c/**'] }], + }), + )?.relatedPaths, + ).toEqual(['src/a1c/x.ts', 'src/a2c/y.ts']); + }); + + // Permission-based failure injection is meaningless to root, and chmod(0) + // does not block reads on Windows — the repo convention for this case. + const isRoot = process.platform === 'win32' || process.getuid?.() === 0; + + it.skipIf(isRoot)( + 'fails closed when a scanned directory cannot be read', + () => { + // An unreadable subtree must fail the review closed, the way the + // identity reader does — silently omitting it degrades the scan into + // a complete-looking result with a hole in it. + const worktree = temp(); + write(join(worktree, 'src', 'safe.ts')); + const locked = join(worktree, 'src', 'locked'); + mkdirSync(locked); + writeFileSync(join(locked, 'inner.ts'), ''); + chmodSync(locked, 0); + try { + expect(() => + provide( + worktree, + ['src/change.ts'], + manifest({ + rules: [{ paths: ['src/**'], relatedPaths: ['src/**'] }], + }), + ), + ).toThrow(); + } finally { + chmodSync(locked, 0o755); + } + }, + ); +}); diff --git a/packages/cli/src/commands/review/lib/manifest-repository-context.ts b/packages/cli/src/commands/review/lib/manifest-repository-context.ts new file mode 100644 index 00000000000..5e7a05fc847 --- /dev/null +++ b/packages/cli/src/commands/review/lib/manifest-repository-context.ts @@ -0,0 +1,617 @@ +/** + * @license + * Copyright 2026 Qwen Team + * SPDX-License-Identifier: Apache-2.0 + */ + +import { lstatSync, readdirSync, realpathSync, statSync } from 'node:fs'; +import { isAbsolute, relative, resolve, sep } from 'node:path'; +import type { RepositoryContextRoleId } from './agent-briefs.js'; +import { isRepositoryContextRoleId } from './agent-briefs.js'; +import type { + RepositoryContext, + RepositoryContextProvider, +} from './repository-context.js'; +import { + compareText, + isControlFree, + isSafeRepositoryRelativePath, + MAX_ARRAY_ITEMS, + MAX_IDENTITY_BYTES, + MAX_LABEL_LENGTH, + MAX_NOTE_LENGTH, + MAX_PATH_LENGTH, + MAX_TOKEN_LENGTH, + validateBoundedString, + validateBoundedStringArray, + validateRepositoryContext, +} from './repository-context.js'; + +const MANIFEST_PATH = '.qwen/review-context.json'; +/** + * Visited-entry ceiling for one `relatedPaths` expansion, counted across all + * scan roots. Dependency and build-output trees (SKIPPED_DIRECTORIES) are + * never descended into, so only source-bearing entries count. Calibrated on + * this repository: the whole `packages/` tree of an installed checkout stays + * under it, so a honestly scoped manifest never fails a review, while a + * pathological scan still ends. Exceeded, the provider throws (fail closed), + * like every other manifest error. + */ +export const MAX_GLOB_CANDIDATES = 16384; +/** + * Matching-work ceiling for one stage. The visited-entry cap bounds a + * COUNT; the per-candidate matching work is a separate dimension — one + * memoised `**` match is quadratic in segment LENGTH, and in an untrusted + * repository an attacker controls both lengths within their schema maxima + * (255-byte filenames, 512-character patterns), so billing segment COUNTS + * never trips for a schema-legal stall shape. Every attempted pattern match + * therefore charges `pattern.length × path.length` against this budget, in + * the rule filter as well as the expansion, so a matching burst fails + * closed instead of stalling the step. Calibrated so the documented + * legitimate scan — every entry of an installed-checkout `packages/` tree + * against a handful of realistic globs — stays far below it, while a + * schema-max adversarial evaluation exhausts it within the first few dozen + * candidates. + */ +export const MAX_MATCH_WORK = 1024 * 1024 * 1024; +const MAX_RULES = 128; +const MANIFEST_PREFIX = 'repository context manifest '; + +// Dependency and build-output trees hold orders of magnitude more entries +// than any source subtree and can never be a review target; descending into +// them would exhaust the visited-entry ceiling on every installed checkout. +// Names tracked source must not live under in this repository's conventions +// (a `build/` directory holds real scripts here) stay out of this set. +// Membership is compared case-insensitively at every enforcement site, or a +// case-varied pattern walks into a skipped tree on every platform. +const SKIPPED_DIRECTORIES = new Set([ + '.git', + '.next', + '.turbo', + 'coverage', + 'dist', + 'node_modules', + 'out', + 'target', +]); + +const MANIFEST_KEYS = ['label', 'rules', 'version'].sort(); +const RULE_KEYS = [ + 'domains', + 'paths', + 'recommendedTests', + 'relatedPaths', + 'requiredAgents', + 'requiredConfigurations', + 'unverifiedDimensions', + 'verificationNotes', +].sort(); + +interface ManifestRule { + paths: string[]; + relatedPaths: string[]; + domains: string[]; + recommendedTests: string[]; + requiredConfigurations: string[]; + requiredAgents: RepositoryContextRoleId[]; + unverifiedDimensions: string[]; + verificationNotes: string[]; +} + +interface Manifest { + label: string; + rules: ManifestRule[]; +} + +function hasExactKeys( + value: Record, + expected: string[], +): boolean { + const keys = Object.keys(value).sort(); + return ( + keys.length === expected.length && + keys.every((key, index) => key === expected[index]) + ); +} + +function validateManifestString( + value: unknown, + field: string, + maxLength: number, +): asserts value is string { + validateBoundedString(value, field, maxLength, MANIFEST_PREFIX); +} + +/** + * Manifest arrays are human-authored, so they need only be UNIQUE — hand- + * sorting a config file is a sharp edge that would fail whole reviews over + * cosmetics. The provider merges and sorts before the wire format's strict + * sorted-and-unique validator ever sees the result (see sortedUnique below). + */ +function validateManifestStringArray( + value: unknown, + field: string, + maxLength: number, +): asserts value is string[] { + validateBoundedStringArray(value, field, maxLength, MANIFEST_PREFIX); + if (new Set(value).size !== value.length) { + throw new Error(`${MANIFEST_PREFIX}${field} must not contain duplicates`); + } +} + +function validateGlob(pattern: string, field: string): void { + const segments = pattern.split('/'); + if ( + pattern.length > MAX_PATH_LENGTH || + !isControlFree(pattern) || + pattern.startsWith('/') || + /^[A-Za-z]:/.test(pattern) || + pattern.startsWith('!') || + pattern.includes('\\') || + /[{}[\]()]/.test(pattern) || + segments.some( + (segment) => + segment === '' || + segment === '.' || + segment === '..' || + (segment.includes('**') && segment !== '**'), + ) + ) { + throw new Error( + `repository context manifest ${field} contains unsafe glob`, + ); + } + // The never-descend invariant below is enforced for entries discovered + // during recursion; a pattern rooted inside a skipped tree would bypass + // it through the scan roots, so such patterns are rejected here too. + if ( + segments.some((segment) => SKIPPED_DIRECTORIES.has(segment.toLowerCase())) + ) { + throw new Error( + `repository context manifest ${field} enters a skipped directory`, + ); + } +} + +function validateGlobArray( + value: unknown, + field: string, + requireDirectoryPrefix: boolean, +): asserts value is string[] { + validateManifestStringArray(value, field, MAX_PATH_LENGTH); + for (const pattern of value) { + validateGlob(pattern, field); + // A wildcard glob must start below a non-wildcard directory segment so + // expansion cannot begin with a repository-wide wildcard. A completely + // static entry can never start with one, so it may sit at the top level + // and resolves to itself when it exists as a regular file. + if (requireDirectoryPrefix && /[*?]/.test(pattern.split('/')[0])) { + throw new Error( + `repository context manifest ${field} requires a directory prefix`, + ); + } + } +} + +function optionalStringArray( + rule: Record, + field: keyof Omit, + maxLength: number, +): string[] { + const value = rule[field]; + if (value === undefined) return []; + validateManifestStringArray(value, field, maxLength); + return value; +} + +function parseManifest(content: string): Manifest { + if (content.length > MAX_IDENTITY_BYTES) { + throw new Error('repository context manifest exceeds the size limit'); + } + let value: unknown; + try { + value = JSON.parse(content); + } catch { + throw new Error('repository context manifest is not valid JSON'); + } + if ( + typeof value !== 'object' || + value === null || + Array.isArray(value) || + !hasExactKeys(value as Record, MANIFEST_KEYS) + ) { + throw new Error( + 'repository context manifest has unknown or missing fields', + ); + } + const manifest = value as Record; + if (manifest['version'] !== 1) { + throw new Error('unsupported repository context manifest version'); + } + validateManifestString(manifest['label'], 'label', MAX_LABEL_LENGTH); + if ( + !Array.isArray(manifest['rules']) || + manifest['rules'].length > MAX_RULES + ) { + throw new Error('repository context manifest rules is invalid'); + } + + const rules = manifest['rules'].map((value, index): ManifestRule => { + if (typeof value !== 'object' || value === null || Array.isArray(value)) { + throw new Error(`repository context manifest rules[${index}] is invalid`); + } + const rule = value as Record; + const keys = Object.keys(rule).sort(); + if ( + !keys.includes('paths') || + keys.some((key) => !RULE_KEYS.includes(key)) + ) { + throw new Error( + `repository context manifest rules[${index}] has unknown or missing fields`, + ); + } + validateGlobArray(rule['paths'], `rules[${index}].paths`, false); + + const relatedPaths = rule['relatedPaths']; + if (relatedPaths !== undefined) { + validateGlobArray(relatedPaths, `rules[${index}].relatedPaths`, true); + } + const requiredAgents = rule['requiredAgents']; + if (requiredAgents !== undefined) { + validateManifestStringArray( + requiredAgents, + `rules[${index}].requiredAgents`, + MAX_TOKEN_LENGTH, + ); + if (requiredAgents.some((role) => !isRepositoryContextRoleId(role))) { + throw new Error( + `repository context manifest rules[${index}].requiredAgents contains an unsupported role`, + ); + } + } + + return { + paths: rule['paths'], + relatedPaths: relatedPaths ?? [], + domains: optionalStringArray(rule, 'domains', MAX_TOKEN_LENGTH), + recommendedTests: optionalStringArray( + rule, + 'recommendedTests', + MAX_TOKEN_LENGTH, + ), + requiredConfigurations: optionalStringArray( + rule, + 'requiredConfigurations', + MAX_TOKEN_LENGTH, + ), + requiredAgents: (requiredAgents ?? []) as RepositoryContextRoleId[], + unverifiedDimensions: optionalStringArray( + rule, + 'unverifiedDimensions', + MAX_NOTE_LENGTH, + ), + verificationNotes: optionalStringArray( + rule, + 'verificationNotes', + MAX_NOTE_LENGTH, + ), + }; + }); + + // Bound the rule-matching work fail-closed: the filter tests every changed + // path against every `paths` pattern, so the total — not just each rule's + // array — is what a bulk-change PR multiplies against. + const totalPathPatterns = rules.reduce( + (sum, rule) => sum + rule.paths.length, + 0, + ); + if (totalPathPatterns > MAX_ARRAY_ITEMS) { + throw new Error(`${MANIFEST_PREFIX}paths exceeds limit`); + } + + return { label: manifest['label'], rules }; +} + +// Polynomial wildcard match for one segment. The backtracking regex this +// replaced went exponential on a segment with several `*` — a legal manifest +// pattern plus a legal long filename hangs one `test()` call effectively +// forever, and in an untrusted repository both halves are attacker-committed. +function segmentMatches(pattern: string, value: string): boolean { + let patternIndex = 0; + let valueIndex = 0; + let starIndex = -1; + let markIndex = 0; + while (valueIndex < value.length) { + const character = pattern[patternIndex]; + if (character === '?' || character === value[valueIndex]) { + patternIndex++; + valueIndex++; + } else if (character === '*') { + starIndex = patternIndex; + markIndex = valueIndex; + patternIndex++; + } else if (starIndex !== -1) { + patternIndex = starIndex + 1; + markIndex++; + valueIndex = markIndex; + } else { + return false; + } + } + while (pattern[patternIndex] === '*') patternIndex++; + return patternIndex === pattern.length; +} + +function globMatches(pattern: string, path: string): boolean { + const patternSegments = pattern.split('/'); + const pathSegments = path.split('/'); + const memo = new Map(); + const matches = (patternIndex: number, pathIndex: number): boolean => { + const key = `${patternIndex}:${pathIndex}`; + const cached = memo.get(key); + if (cached !== undefined) return cached; + + let result: boolean; + if (patternIndex === patternSegments.length) { + result = pathIndex === pathSegments.length; + } else if (patternSegments[patternIndex] === '**') { + result = + matches(patternIndex + 1, pathIndex) || + (pathIndex < pathSegments.length && + matches(patternIndex, pathIndex + 1)); + } else { + result = + pathIndex < pathSegments.length && + segmentMatches( + patternSegments[patternIndex], + pathSegments[pathIndex], + ) && + matches(patternIndex + 1, pathIndex + 1); + } + memo.set(key, result); + return result; + }; + return matches(0, 0); +} + +function isContainedFile(worktree: string, path: string): boolean { + try { + const resolved = realpathSync(resolve(worktree, path)); + const contained = relative(worktree, resolved); + return ( + contained !== '' && + !isAbsolute(contained) && + contained !== '..' && + !contained.startsWith(`..${sep}`) && + statSync(resolved).isFile() + ); + } catch { + return false; + } +} + +function staticDirectoryPrefix(pattern: string): string { + const prefix: string[] = []; + for (const segment of pattern.split('/')) { + if (segment.includes('*') || segment.includes('?')) break; + prefix.push(segment); + } + return prefix.join('/'); +} + +function minimalScanRoots(patterns: readonly string[]): string[] { + const roots = sortedUnique(patterns.map(staticDirectoryPrefix)); + return roots.filter( + (root, index) => + !roots.some( + (candidate, candidateIndex) => + candidateIndex !== index && root.startsWith(`${candidate}/`), + ), + ); +} + +function expandRelatedPaths( + worktree: string, + patterns: readonly string[], + changedPaths: ReadonlySet, +): string[] { + const matches = new Set(); + let candidates = 0; + const patternLengths = patterns.map((pattern) => pattern.length); + let matchWork = 0; + + const anyPatternMatches = (path: string): boolean => { + for (let index = 0; index < patterns.length; index++) { + matchWork += patternLengths[index] * path.length; + if (matchWork > MAX_MATCH_WORK) { + throw new Error( + 'repository context manifest relatedPaths matching work exceeds limit', + ); + } + if (globMatches(patterns[index], path)) return true; + } + return false; + }; + + const visit = (directory: string): void => { + let entries; + try { + const stat = lstatSync(resolve(worktree, directory)); + if (stat.isSymbolicLink()) return; + if (!stat.isDirectory()) { + candidates++; + if (candidates > MAX_GLOB_CANDIDATES) { + throw new Error( + 'repository context manifest relatedPaths scan exceeds limit', + ); + } + const path = directory; + if ( + !changedPaths.has(path) && + isContainedFile(worktree, path) && + anyPatternMatches(path) + ) { + matches.add(path); + if (matches.size > MAX_ARRAY_ITEMS) { + throw new Error( + 'repository context manifest relatedPaths exceeds limit', + ); + } + } + return; + } + entries = readdirSync(resolve(worktree, directory), { + withFileTypes: true, + }); + // Bound the listing before sorting it, or an oversized directory pays + // the full read plus an O(n log n) sort before the cap can trip. + if (candidates + entries.length > MAX_GLOB_CANDIDATES) { + throw new Error( + 'repository context manifest relatedPaths scan exceeds limit', + ); + } + entries.sort((left, right) => compareText(left.name, right.name)); + } catch (error) { + if ( + error instanceof Error && + error.message.startsWith('repository context manifest') + ) { + throw error; + } + // A subtree that EXISTS but cannot be read fails the review closed, + // the way the identity reader does — silently skipping it would + // degrade the scan into a complete-looking result with a hole in it. + // Only a racing deletion still reads as "absent". + const code = (error as NodeJS.ErrnoException | null)?.code; + if (code !== 'ENOENT' && code !== 'ENOTDIR') throw error; + return; + } + + for (const entry of entries) { + candidates++; + if (candidates > MAX_GLOB_CANDIDATES) { + throw new Error( + 'repository context manifest relatedPaths scan exceeds limit', + ); + } + if (entry.isSymbolicLink()) continue; + const path = `${directory}/${entry.name}`; + if (entry.isDirectory()) { + if (!SKIPPED_DIRECTORIES.has(entry.name.toLowerCase())) visit(path); + continue; + } + // Disk names can carry POSIX-legal bytes the wire format rejects (a + // backslash, control characters); skip them like changedPaths does + // instead of failing the whole review over one odd filename. The + // shape gates run before matching: an over-long path can never reach + // the wire format, so it can never match, and the cheap checks keep + // the memoised matcher away from disk garbage. + if ( + !entry.isFile() || + changedPaths.has(path) || + !isSafeRepositoryRelativePath(path) || + !isContainedFile(worktree, path) || + !anyPatternMatches(path) + ) { + continue; + } + matches.add(path); + if (matches.size > MAX_ARRAY_ITEMS) { + throw new Error( + 'repository context manifest relatedPaths exceeds limit', + ); + } + } + }; + + for (const root of minimalScanRoots(patterns)) visit(root); + return [...matches].sort(compareText); +} + +function sortedUnique(values: Iterable): string[] { + return [...new Set(values)].sort(compareText); +} + +/** + * The merge of all matching rules can outgrow the wire bound even when every + * single rule honors it; cap it here so the error names the manifest instead + * of surfacing later as a shape error from the wire validator. `subject` + * distinguishes the merged PATTERN list of `relatedPaths` from the + * resolved-files cap in the expansion, which reports the same field name — + * an operator diagnosing a fail-closed step must be able to tell whether to + * trim the manifest's glob list or narrow the globs' reach. + */ +function cappedSortedUnique(values: string[], subject: string): string[] { + const merged = sortedUnique(values); + if (merged.length > MAX_ARRAY_ITEMS) { + throw new Error(`${MANIFEST_PREFIX}${subject} exceeds limit`); + } + return merged; +} + +export const manifestRepositoryContextProvider: RepositoryContextProvider = { + provide(input) { + const content = input.readIdentityFile(MANIFEST_PATH); + if (content === null) return null; + const manifest = parseManifest(content); + // Nothing caps the changed-path count a bulk diff brings, so the filter + // charges the same budget the expansion does, with the same length-based + // billing — a matching burst must fail closed in this stage too instead + // of stalling the step. + let filterWork = 0; + const matched = manifest.rules.filter((rule) => + input.changedPaths.some((path) => + rule.paths.some((pattern) => { + filterWork += pattern.length * path.length; + if (filterWork > MAX_MATCH_WORK) { + throw new Error( + `${MANIFEST_PREFIX}paths matching work exceeds limit`, + ); + } + return globMatches(pattern, path); + }), + ), + ); + if (matched.length === 0) return null; + + const changedPaths = new Set(input.changedPaths); + const context: RepositoryContext = { + version: 1, + provider: 'manifest', + label: manifest.label, + domains: cappedSortedUnique( + matched.flatMap((rule) => rule.domains), + 'domains', + ), + relatedPaths: expandRelatedPaths( + input.worktree, + cappedSortedUnique( + matched.flatMap((rule) => rule.relatedPaths), + 'relatedPaths glob list', + ), + changedPaths, + ), + recommendedTests: cappedSortedUnique( + matched.flatMap((rule) => rule.recommendedTests), + 'recommendedTests', + ), + requiredConfigurations: cappedSortedUnique( + matched.flatMap((rule) => rule.requiredConfigurations), + 'requiredConfigurations', + ), + requiredAgents: cappedSortedUnique( + matched.flatMap((rule) => rule.requiredAgents), + 'requiredAgents', + ) as RepositoryContextRoleId[], + unverifiedDimensions: cappedSortedUnique( + matched.flatMap((rule) => rule.unverifiedDimensions), + 'unverifiedDimensions', + ), + verificationNotes: cappedSortedUnique( + matched.flatMap((rule) => rule.verificationNotes), + 'verificationNotes', + ), + }; + return validateRepositoryContext(context); + }, +}; diff --git a/packages/cli/src/commands/review/lib/report.ts b/packages/cli/src/commands/review/lib/report.ts index 46bb625442d..3991fa496f6 100644 --- a/packages/cli/src/commands/review/lib/report.ts +++ b/packages/cli/src/commands/review/lib/report.ts @@ -14,6 +14,7 @@ import { writeStderrLine } from '../../../utils/stdioHelpers.js'; import { classifyHeavy } from './heavy.js'; import type { DiffChunk, DiffPlan, PathKind } from './diff-plan.js'; import { reviewBudget, type ReviewBudget } from './budget.js'; +import type { RepositoryContext } from './repository-context.js'; export interface FileMetric { path: string; @@ -96,6 +97,7 @@ export interface PlanReport { * roster's job, and the roster reads `effort`. */ budget: ReviewBudget; + repositoryContext?: RepositoryContext; } /** diff --git a/packages/cli/src/commands/review/lib/repository-context.test.ts b/packages/cli/src/commands/review/lib/repository-context.test.ts new file mode 100644 index 00000000000..236d6a02e5c --- /dev/null +++ b/packages/cli/src/commands/review/lib/repository-context.test.ts @@ -0,0 +1,340 @@ +/** + * @license + * Copyright 2026 Qwen Team + * SPDX-License-Identifier: Apache-2.0 + */ + +import { describe, expect, it } from 'vitest'; +import { REPOSITORY_CONTEXT_ROLES } from './agent-briefs.js'; +import { + repositoryContextOf, + validateRepositoryContext, +} from './repository-context.js'; + +const valid = { + version: 1, + provider: 'example-provider', + label: 'Example project', + domains: ['compiler', 'runtime'], + relatedPaths: ['src/compiler.ts', 'src/runtime.ts'], + recommendedTests: ['test:compiler', 'test:runtime'], + requiredConfigurations: ['debug', 'linux-x64'], + requiredAgents: ['1a', 'test-matrix'], + unverifiedDimensions: ['Alternate runtime was not exercised'], + verificationNotes: ['Use the repository native test runner'], +}; + +describe('repository context validation', () => { + it('accepts the strict versioned generic schema', () => { + expect(validateRepositoryContext(valid)).toEqual(valid); + expect(repositoryContextOf({ repositoryContext: valid })).toEqual(valid); + expect(repositoryContextOf({})).toBeNull(); + }); + + it('fails closed on a present-but-null repositoryContext', () => { + // repo-context writes literal `null` artifact files, so a corrupted plan + // can carry the shape; a falsy-check regression would degrade open in + // every consumer at once instead of failing closed. + expect(() => repositoryContextOf({ repositoryContext: null })).toThrow( + 'repositoryContext must be an object', + ); + }); + + it('rejects unknown or missing fields and versions', () => { + expect(() => validateRepositoryContext({ ...valid, version: 2 })).toThrow( + 'unsupported repositoryContext version', + ); + expect(() => validateRepositoryContext({ ...valid, extra: true })).toThrow( + 'unknown or missing fields', + ); + const { label: _label, ...withoutLabel } = valid; + expect(() => validateRepositoryContext(withoutLabel)).toThrow( + 'unknown or missing fields', + ); + }); + + it('accepts bounded Unicode text and repository paths with spaces', () => { + const context = { + ...valid, + label: '示例仓库', + domains: ['编译器', '运行时'], + relatedPaths: ['docs/设计说明.md', 'src/generated files/output.ts'], + recommendedTests: ['运行核心测试'], + requiredConfigurations: ['调试模式'], + unverifiedDimensions: ['未验证备用运行时'], + verificationNotes: ['使用仓库原生测试命令'], + }; + expect(validateRepositoryContext(context)).toEqual(context); + }); + + it('requires bounded sorted unique safe tokens and text', () => { + expect(() => + validateRepositoryContext({ ...valid, domains: ['runtime', 'compiler'] }), + ).toThrow('sorted and unique'); + expect(() => + validateRepositoryContext({ ...valid, domains: ['runtime', 'runtime'] }), + ).toThrow('sorted and unique'); + expect(() => + validateRepositoryContext({ ...valid, provider: '../provider' }), + ).toThrow('provider is invalid'); + // isControlFree rejects all of 0x00-0x1F, 0x7F-0x9F, U+2028/2029, the + // bidi directional formatting block, and zero-width hiding characters; + // probing range ends plus interior points pins the range, not a + // four-separator regex (under which `label: 'X\r## heading'` + // validates and CR-overwrites the rendered heading). + for (const separator of [ + '\u0000', + '\u0005', + '\r', + '\u001f', + '\n', + '\u007f', + '\u0085', + '\u009f', + '\u2028', + '\u2029', + '\u061c', + '\u200b', + '\u200e', + '\u200f', + '\u202a', + '\u202e', + '\u2066', + '\u2069', + '\ufeff', + ]) { + expect(() => + validateRepositoryContext({ + ...valid, + label: `bad${separator}heading`, + }), + ).toThrow('label is invalid'); + } + expect(() => + validateRepositoryContext({ + ...valid, + verificationNotes: ['x'.repeat(513)], + }), + ).toThrow('verificationNotes is invalid'); + expect(() => + validateRepositoryContext({ + ...valid, + domains: Array.from({ length: 129 }, (_, index) => `d${index}`), + }), + ).toThrow('domains is invalid'); + }); + + it('enforces sorted-and-unique on every array field', () => { + // The manifest provider pre-sorts today; a future provider or a + // hand-edited plan would not, so the wire check is pinned per field. + const probes: Record = { + recommendedTests: [ + ['test:runtime', 'test:compiler'], + ['test:compiler', 'test:compiler'], + ], + requiredConfigurations: [ + ['linux-x64', 'debug'], + ['debug', 'debug'], + ], + relatedPaths: [ + ['src/runtime.ts', 'src/compiler.ts'], + ['src/compiler.ts', 'src/compiler.ts'], + ], + requiredAgents: [ + ['test-matrix', '1a'], + ['test-matrix', 'test-matrix'], + ], + unverifiedDimensions: [ + ['later boundary', 'earlier boundary'], + ['same boundary', 'same boundary'], + ], + verificationNotes: [ + ['second note', 'first note'], + ['same note', 'same note'], + ], + }; + for (const [field, [unsorted, duplicated]] of Object.entries(probes)) { + expect(() => + validateRepositoryContext({ ...valid, [field]: unsorted }), + ).toThrow(`${field} must be sorted and unique`); + expect(() => + validateRepositoryContext({ ...valid, [field]: duplicated }), + ).toThrow(`${field} must be sorted and unique`); + } + }); + + it('accepts every length bound exactly and rejects one past it', () => { + // provider 64, label 120, token 160, path 512, note 512: the accept side + // pins `>` (a `>=` regression would reject manifests at the documented + // bounds) and the reject side pins the four remaining constants. + const atBound = { + ...valid, + provider: 'p'.repeat(64), + label: 'l'.repeat(120), + domains: ['t'.repeat(160)], + relatedPaths: ['a'.repeat(512)], + recommendedTests: ['t'.repeat(160)], + requiredConfigurations: ['t'.repeat(160)], + unverifiedDimensions: ['n'.repeat(512)], + verificationNotes: ['n'.repeat(512)], + }; + expect(validateRepositoryContext(atBound)).toEqual(atBound); + + expect(() => + validateRepositoryContext({ ...valid, provider: 'p'.repeat(65) }), + ).toThrow('provider is invalid'); + expect(() => + validateRepositoryContext({ ...valid, label: 'l'.repeat(121) }), + ).toThrow('label is invalid'); + expect(() => + validateRepositoryContext({ ...valid, domains: ['t'.repeat(161)] }), + ).toThrow('domains is invalid'); + expect(() => + validateRepositoryContext({ + ...valid, + relatedPaths: ['a'.repeat(513)], + }), + ).toThrow('relatedPaths is invalid'); + expect(() => + validateRepositoryContext({ + ...valid, + recommendedTests: ['t'.repeat(161)], + }), + ).toThrow('recommendedTests is invalid'); + expect(() => + validateRepositoryContext({ + ...valid, + requiredConfigurations: ['t'.repeat(161)], + }), + ).toThrow('requiredConfigurations is invalid'); + }); + + it('accepts the item-count bound exactly', () => { + // The reject side pins 129 items; this accept pin sits exactly at + // MAX_ARRAY_ITEMS, where a `>` → `>=` regression would reject the + // maximum valid manifest at the documented bound. + const atBound = { + ...valid, + domains: Array.from( + { length: 128 }, + (_, index) => `d-${String(index).padStart(3, '0')}`, + ), + }; + expect(validateRepositoryContext(atBound)).toEqual(atBound); + }); + + it('accepts every role the allow-list admits', () => { + // Hardcoded, not spread from the constant: the accept side must pin + // all 13 roles, or dropping one from REPOSITORY_CONTEXT_ROLES ships + // green (`satisfies readonly RoleId[]` still compiles, the type + // narrows silently) and every consumer fails closed on a valid + // manifest's required agent. + const allRoles = [ + '1a', + '1b', + '1c', + '2', + '3a', + '3b', + '3c', + '4', + '5', + '6a', + '6b', + '6c', + 'test-matrix', + ]; + expect([...REPOSITORY_CONTEXT_ROLES]).toEqual(allRoles); + const context = { ...valid, requiredAgents: allRoles }; + expect(validateRepositoryContext(context)).toEqual(context); + }); + + it('rejects control characters inside array items', () => { + for (const field of [ + 'domains', + 'relatedPaths', + 'unverifiedDimensions', + 'verificationNotes', + ] as const) { + for (const separator of [ + '\u0000', + '\u0005', + '\r', + '\u001f', + '\n', + '\u007f', + '\u0085', + '\u009f', + '\u2028', + '\u2029', + '\u061c', + '\u200b', + '\u200e', + '\u200f', + '\u202a', + '\u202e', + '\u2066', + '\u2069', + '\ufeff', + ]) { + expect(() => + validateRepositoryContext({ + ...valid, + [field]: [`bad${separator}item`], + }), + ).toThrow(`${field} is invalid`); + } + } + }); + + it('fails closed on non-string and empty values', () => { + // The string-type and non-empty gates fail closed today; pin them, or + // a future simplification ships green and `[object Object]` / `123` / + // empty entries flow into every reviewer prompt and the posted body. + expect(() => validateRepositoryContext({ ...valid, label: 123 })).toThrow( + 'label is invalid', + ); + expect(() => validateRepositoryContext({ ...valid, label: '' })).toThrow( + 'label is invalid', + ); + expect(() => + validateRepositoryContext({ ...valid, domains: [123] }), + ).toThrow('domains is invalid'); + expect(() => + validateRepositoryContext({ ...valid, domains: [''] }), + ).toThrow('domains is invalid'); + expect(() => + validateRepositoryContext({ ...valid, verificationNotes: [null] }), + ).toThrow('verificationNotes is invalid'); + }); + + it('rejects unsafe paths and roles that cannot join the initial roster', () => { + for (const path of [ + '../secret', + '/absolute', + 'C:', + 'C:relative', + 'C:/absolute', + 'd:relative', + 'a//b', + 'a/./b', + 'a\\b', + 'a/../b', + ]) { + expect(() => + validateRepositoryContext({ ...valid, relatedPaths: [path] }), + ).toThrow(); + } + for (const role of [ + 'not-a-role', + '7', + 'invariant-a', + 'verify', + 'reverse-audit', + ]) { + expect(() => + validateRepositoryContext({ ...valid, requiredAgents: [role] }), + ).toThrow('unsupported role'); + } + }); +}); diff --git a/packages/cli/src/commands/review/lib/repository-context.ts b/packages/cli/src/commands/review/lib/repository-context.ts new file mode 100644 index 00000000000..c85b059f148 --- /dev/null +++ b/packages/cli/src/commands/review/lib/repository-context.ts @@ -0,0 +1,275 @@ +/** + * @license + * Copyright 2026 Qwen Team + * SPDX-License-Identifier: Apache-2.0 + */ + +import type { RepositoryContextRoleId } from './agent-briefs.js'; +import { isRepositoryContextRoleId } from './agent-briefs.js'; + +export const REPOSITORY_CONTEXT_VERSION = 1 as const; + +// Shared bounds for the context contract and every provider that produces one: +// a provider validator must emit exactly what validateRepositoryContext accepts, +// so both read the same constants instead of keeping lockstep copies that can +// drift. +export const MAX_ARRAY_ITEMS = 128; +const MAX_PROVIDER_LENGTH = 64; +export const MAX_LABEL_LENGTH = 120; +export const MAX_TOKEN_LENGTH = 160; +export const MAX_PATH_LENGTH = 512; +export const MAX_NOTE_LENGTH = 512; + +/** + * Fail-closed ceiling on a single identity read. An identity file is a small + * marker or manifest; a multi-megabyte one is an attacker payload whose cost + * lands in `JSON.parse` BEFORE any schema validation can reject it (the + * parse runs first). Both the worktree reader (stat size) and the parser + * (content length) enforce this, symmetric in both modes. One megabyte is + * far beyond any honest manifest and far below the heap damage a + * near-push-limit file demonstrably causes. + */ +export const MAX_IDENTITY_BYTES = 1024 * 1024; + +export interface RepositoryContext { + version: typeof REPOSITORY_CONTEXT_VERSION; + provider: string; + label: string; + domains: string[]; + relatedPaths: string[]; + recommendedTests: string[]; + requiredConfigurations: string[]; + requiredAgents: RepositoryContextRoleId[]; + unverifiedDimensions: string[]; + verificationNotes: string[]; +} + +export interface RepositoryContextPlan { + repositoryContext?: unknown; +} + +export interface RepositoryContextProviderInput { + worktree: string; + changedPaths: string[]; + /** + * Read an identity file the provider keys on. The content is identical in + * every mode — CRLF normalised to LF, surrounding whitespace trimmed — so a + * provider that exact-compares a marker file gets the same value in a pull + * request review (read from the trusted merge base) and a local one (read + * from the worktree). `null` means the file is absent; a read failure + * THROWS, fail-closed, so a broken read cannot pose as "not this + * repository". + */ + readIdentityFile(relativePath: string): string | null; +} + +export interface RepositoryContextProvider { + provide(input: RepositoryContextProviderInput): RepositoryContext | null; +} + +const CONTEXT_KEYS = [ + 'version', + 'provider', + 'label', + 'domains', + 'relatedPaths', + 'recommendedTests', + 'requiredConfigurations', + 'requiredAgents', + 'unverifiedDimensions', + 'verificationNotes', +].sort(); + +const SAFE_PROVIDER = /^[a-z0-9][a-z0-9._-]*$/; + +// Unicode bidi directional formatting code points render attacker-controlled +// text in a different order than its bytes, and the zero-width characters +// here hide content the eye never sees (trojan-source class display +// spoofing). A manifest entry carrying one survives the provider and is +// rendered intact into the posted review body and every reviewer prompt, so +// the validator rejects them. Zero-width JOINERS (0x200C/0x200D) stay +// allowed: they carry emoji ZWJ sequences and several living scripts, and +// neither reorder nor hide on their own. +const SPOOFING_FORMAT_CODE_UNITS = (code: number): boolean => + code === 0x61c || + code === 0x200b || + code === 0x200e || + code === 0x200f || + (code >= 0x202a && code <= 0x202e) || + (code >= 0x2066 && code <= 0x2069) || + code === 0xfeff; + +export function isControlFree(value: string): boolean { + for (let index = 0; index < value.length; index++) { + const code = value.charCodeAt(index); + if ( + code < 0x20 || + (code >= 0x7f && code <= 0x9f) || + code === 0x2028 || + code === 0x2029 || + SPOOFING_FORMAT_CODE_UNITS(code) + ) { + return false; + } + } + return true; +} + +export function compareText(left: string, right: string): number { + return left < right ? -1 : left > right ? 1 : 0; +} + +function isSortedUnique(values: readonly string[]): boolean { + return values.every( + (value, index) => index === 0 || compareText(values[index - 1], value) < 0, + ); +} + +export function isSafeRepositoryRelativePath(path: string): boolean { + const segments = path.split('/'); + return ( + path.length > 0 && + path.length <= MAX_PATH_LENGTH && + isControlFree(path) && + !path.startsWith('/') && + !/^[A-Za-z]:/.test(path) && + !path.includes('\\') && + segments.every( + (segment) => segment !== '' && segment !== '.' && segment !== '..', + ) + ); +} + +/** + * Bounded, non-empty, control-character-free string. `prefix` names the owner + * of the field in the error (`repositoryContext.` for the wire format, the + * manifest's own wording for manifest parsing), so one validator serves both + * without their bounds drifting apart. + */ +export function validateBoundedString( + value: unknown, + field: string, + maxLength: number, + prefix: string, + pattern?: RegExp, +): asserts value is string { + if ( + typeof value !== 'string' || + value.length === 0 || + value.length > maxLength || + !isControlFree(value) || + (pattern !== undefined && !pattern.test(value)) + ) { + throw new Error(`${prefix}${field} is invalid`); + } +} + +/** Item-shape half of {@link validateBoundedString}; ordering is the caller's. */ +export function validateBoundedStringArray( + value: unknown, + field: string, + maxLength: number, + prefix: string, + pattern?: RegExp, +): asserts value is string[] { + if ( + !Array.isArray(value) || + value.length > MAX_ARRAY_ITEMS || + value.some( + (item) => + typeof item !== 'string' || + item.length === 0 || + item.length > maxLength || + !isControlFree(item) || + (pattern !== undefined && !pattern.test(item)), + ) + ) { + throw new Error(`${prefix}${field} is invalid`); + } +} + +/** Validate repository context before any downstream consumer trusts it. */ +export function validateRepositoryContext(value: unknown): RepositoryContext { + if (typeof value !== 'object' || value === null || Array.isArray(value)) { + throw new Error('repositoryContext must be an object'); + } + const context = value as Record; + const keys = Object.keys(context).sort(); + if ( + keys.length !== CONTEXT_KEYS.length || + keys.some((key, index) => key !== CONTEXT_KEYS[index]) + ) { + throw new Error('repositoryContext has unknown or missing fields'); + } + if (context['version'] !== REPOSITORY_CONTEXT_VERSION) { + throw new Error('unsupported repositoryContext version'); + } + + const prefix = 'repositoryContext.'; + // The wire format every downstream consumer trusts is deterministic by + // construction: sorted and unique. Providers earn that shape on the way in + // (the manifest provider sorts and dedupes); the validator enforces it. + const requireSortedUnique = (values: readonly string[], field: string) => { + if (!isSortedUnique(values)) { + throw new Error(`${prefix}${field} must be sorted and unique`); + } + }; + + validateBoundedString( + context['provider'], + 'provider', + MAX_PROVIDER_LENGTH, + prefix, + SAFE_PROVIDER, + ); + validateBoundedString(context['label'], 'label', MAX_LABEL_LENGTH, prefix); + for (const field of [ + 'domains', + 'recommendedTests', + 'requiredConfigurations', + ] as const) { + validateBoundedStringArray(context[field], field, MAX_TOKEN_LENGTH, prefix); + requireSortedUnique(context[field], field); + } + validateBoundedStringArray( + context['relatedPaths'], + 'relatedPaths', + MAX_PATH_LENGTH, + prefix, + ); + requireSortedUnique(context['relatedPaths'], 'relatedPaths'); + if ( + context['relatedPaths'].some((path) => !isSafeRepositoryRelativePath(path)) + ) { + throw new Error('repositoryContext.relatedPaths contains an unsafe path'); + } + // requiredAgents carries no token pattern: the role allow-list membership + // check below is strictly tighter than any shape check. + validateBoundedStringArray( + context['requiredAgents'], + 'requiredAgents', + MAX_TOKEN_LENGTH, + prefix, + ); + requireSortedUnique(context['requiredAgents'], 'requiredAgents'); + if ( + context['requiredAgents'].some((role) => !isRepositoryContextRoleId(role)) + ) { + throw new Error( + 'repositoryContext.requiredAgents contains an unsupported role', + ); + } + for (const field of ['unverifiedDimensions', 'verificationNotes'] as const) { + validateBoundedStringArray(context[field], field, MAX_NOTE_LENGTH, prefix); + requireSortedUnique(context[field], field); + } + + return context as unknown as RepositoryContext; +} + +export function repositoryContextOf( + plan: RepositoryContextPlan, +): RepositoryContext | null { + if (plan.repositoryContext === undefined) return null; + return validateRepositoryContext(plan.repositoryContext); +} diff --git a/packages/cli/src/commands/review/lib/roster.test.ts b/packages/cli/src/commands/review/lib/roster.test.ts index d9fa5182527..d4c3e892ea6 100644 --- a/packages/cli/src/commands/review/lib/roster.test.ts +++ b/packages/cli/src/commands/review/lib/roster.test.ts @@ -165,6 +165,117 @@ describe('requiredAgents — Step 3A', () => { // But there IS a tree, so the tracer and the build still run. expect(keys(local)).toEqual(expect.arrayContaining(['1c', '7'])); }); + + it('requires existing roles recorded by validated repository context without duplicates', () => { + const context = { + version: 1, + provider: 'fake-provider', + label: 'Example project', + domains: ['runtime'], + relatedPaths: ['src/runtime.ts'], + recommendedTests: ['test:runtime'], + requiredConfigurations: ['linux-x64'], + requiredAgents: ['1a', '1b'], + unverifiedDimensions: ['Alternate runtime was not exercised'], + verificationNotes: ['Use the repository native test runner'], + }; + // 1b is policy-permitted but data-gated away here (the diff deletes + // nothing); a context may require it back. 1a is already required and + // must not duplicate. + const roster = keys({ ...PR, repositoryContext: context }); + expect(roster).toContain('1b'); + expect(roster.filter((role) => role === '1a')).toHaveLength(1); + }); + + it('does not let repository context override the effort, topology, or mode gates', () => { + // A manifest may require agents the policy already runs; it may not + // inflate or wedge the run by re-adding roles the policy excludes. + const context = (requiredAgents: string[]) => ({ + version: 1, + provider: 'fake-provider', + label: 'Example project', + domains: [], + relatedPaths: [], + recommendedTests: [], + requiredConfigurations: [], + requiredAgents, + unverifiedDimensions: [], + verificationNotes: [], + }); + + // The adversarial personas are a high-effort dimension: a medium review + // stays medium even when the repository names them. + const medium = keys({ + ...PR, + effort: 'medium', + repositoryContext: context(['6a', '6b', '6c']), + }); + expect(medium).not.toContain('6a'); + expect(medium).not.toContain('6b'); + expect(medium).not.toContain('6c'); + // At high effort the same requirement is honoured (and deduplicated). + expect( + keys({ ...PR, repositoryContext: context(['6a']) }).filter( + (role) => role === '6a', + ), + ).toHaveLength(1); + + // A lightweight review has no tree to grep: 1c cannot be required back. + const light = { + ...PR, + worktreePath: undefined, + prNumber: undefined, + repositoryContext: context(['1c']), + }; + expect(keys(light)).not.toContain('1c'); + + // test-matrix is a fan-out role: a manifest cannot require it into a + // whole-diff (Step 3A) review, whose flow is not built around it — the + // denial half of the gate, which a `return fanOut` → `return true` + // regression would silently drop. + expect( + keys({ ...PR, repositoryContext: context(['test-matrix']) }), + ).not.toContain('test-matrix'); + + // A Step 3B fan-out keeps its topology: whole-diff dimension walkers and + // the high-effort personas stay out, while 3B's own roles are honoured. + const big = { ...PR, srcDiffLines: 5000, diffLines: 6000 }; + const fanOut = keys({ + ...big, + repositoryContext: context(['1b', '2', '6a', 'test-matrix']), + }); + expect(fanOut).not.toContain('2'); + expect(fanOut).not.toContain('6a'); + expect(fanOut.filter((role) => role === 'test-matrix')).toHaveLength(1); + expect(fanOut).toContain('1b'); + }); + + it('fails closed on a present-but-invalid repository context', () => { + // Full wire shape but version 2: the exact-keys check passes, the + // version gate throws. A try/catch-return-null wrapper around + // repositoryContextOf would silently drop every context-required role + // from the roster AND the coverage certification — certifying a run + // where the agents the repository required never launched. + const future = { + version: 2, + provider: 'fake-provider', + label: 'Example project', + domains: [], + relatedPaths: [], + recommendedTests: [], + requiredConfigurations: [], + requiredAgents: [], + unverifiedDimensions: [], + verificationNotes: [], + }; + expect(() => keys({ ...PR, repositoryContext: future })).toThrow( + 'unsupported repositoryContext version', + ); + }); + + it('keeps the generic roster when repository context is absent', () => { + expect(keys(PR)).not.toContain('test-matrix'); + }); }); describe('hasExecutableScript — the script-lint gate predicate', () => { diff --git a/packages/cli/src/commands/review/lib/roster.ts b/packages/cli/src/commands/review/lib/roster.ts index 7d06127151c..3e78adb5794 100644 --- a/packages/cli/src/commands/review/lib/roster.ts +++ b/packages/cli/src/commands/review/lib/roster.ts @@ -24,7 +24,8 @@ // Nothing here is supplied by the caller. A roster the caller could shrink is a // roster that gets shrunk. -import type { RoleId } from './agent-briefs.js'; +import type { RepositoryContextRoleId, RoleId } from './agent-briefs.js'; +import { repositoryContextOf } from './repository-context.js'; import { pathTool } from '../script-lint.js'; /** @@ -72,6 +73,7 @@ export interface RosterPlan { * recomputation then all read the same value and cannot disagree. */ effort?: unknown; + repositoryContext?: unknown; } /** One agent this review must launch. */ @@ -273,5 +275,47 @@ export function requiredAgents(plan: RosterPlan): RequiredAgent[] { } } + const repositoryContext = repositoryContextOf(plan); + for (const role of repositoryContext?.requiredAgents ?? []) { + if (!contextRoleRunsInThisReview(role, plan, mode)) continue; + if (!out.some((agent) => agent.role === role && agent.file === undefined)) { + add(role); + } + } + return out; } + +/** + * A repository context may REQUIRE an agent this review's policy already runs; + * it may not override the policy. The effort gate, the topology split, and the + * mode are cost and capability decisions the roster owns — a manifest naming a + * role they exclude would otherwise silently inflate a medium review with the + * adversarial personas, re-add whole-diff walkers to a chunked 3B fan-out, or + * demand a tree-grepping tracer from a review that has no tree. + */ +function contextRoleRunsInThisReview( + role: RepositoryContextRoleId, + plan: RosterPlan, + mode: ReviewMode, +): boolean { + const fanOut = isTerritoryFanOut(plan); + switch (role) { + case '6a': + case '6b': + case '6c': + return !fanOut && plan.effort !== 'medium'; + case 'test-matrix': + return fanOut; + case '1c': + return mode !== 'diff-only'; + case '1b': + // Both topologies run the removed-behavior audit; whether it has work is + // the diff's business (hasDeletions), not the policy's. + return true; + default: + // Whole-diff dimension walkers exist only in Step 3A; a 3B chunk agent + // already owns every dimension for its own lines. + return !fanOut; + } +} diff --git a/packages/cli/src/commands/review/repo-context.test.ts b/packages/cli/src/commands/review/repo-context.test.ts new file mode 100644 index 00000000000..a10c0d18904 --- /dev/null +++ b/packages/cli/src/commands/review/repo-context.test.ts @@ -0,0 +1,1026 @@ +/** + * @license + * Copyright 2026 Qwen Team + * SPDX-License-Identifier: Apache-2.0 + */ + +import { + chmodSync, + linkSync, + mkdirSync, + mkdtempSync, + readFileSync, + realpathSync, + rmSync, + symlinkSync, + writeFileSync, +} from 'node:fs'; +import { tmpdir } from 'node:os'; +import { dirname, join } from 'node:path'; +import { execFileSync } from 'node:child_process'; +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; +import { + MAX_IDENTITY_BYTES, + type RepositoryContextProvider, +} from './lib/repository-context.js'; +import { repoContextCommand, runRepoContext } from './repo-context.js'; + +const tempRoots: string[] = []; + +function temp(): string { + const root = realpathSync(mkdtempSync(join(tmpdir(), 'repo-context-'))); + tempRoots.push(root); + return root; +} + +function write(path: string, content: string): void { + mkdirSync(dirname(path), { recursive: true }); + writeFileSync(path, content); +} + +function readJson(path: string): unknown { + return JSON.parse(readFileSync(path, 'utf8')); +} + +// Isolate the fixture from the developer's git environment, exactly like +// the sibling review-pipeline git fixtures: a global `commit.gpgsign=true` +// fails the suite for want of a key, and a global `core.hooksPath` runs the +// developer's hooks inside the test commits (`git worktree add` fires +// post-checkout too). The wrappers under test read `process.env` per call. +let savedEnv: NodeJS.ProcessEnv; +let gitHome: string; + +beforeEach(() => { + gitHome = mkdtempSync(join(tmpdir(), 'repo-context-home-')); + writeFileSync(join(gitHome, '.gitconfig'), ''); + savedEnv = { ...process.env }; + process.env['GIT_CONFIG_NOSYSTEM'] = '1'; + process.env['GIT_CONFIG_GLOBAL'] = join(gitHome, '.gitconfig'); + process.env['HOME'] = gitHome; +}); + +afterEach(() => { + process.env = savedEnv; + rmSync(gitHome, { recursive: true, force: true }); + // Every test builds fixture worktrees (several with initialized git + // repos) in the OS tmpdir; leaking them accumulates toward ENOSPC on + // long-lived machines. + for (const root of tempRoots) { + rmSync(root, { recursive: true, force: true }); + } + tempRoots.length = 0; +}); + +function initGit(root: string): void { + execFileSync('git', ['init', '-q', '--template=', root]); + execFileSync('git', ['-C', root, 'config', 'user.email', 'test@example.com']); + execFileSync('git', ['-C', root, 'config', 'user.name', 'Test']); + execFileSync('git', ['-C', root, 'config', 'commit.gpgsign', 'false']); + execFileSync('git', [ + '-C', + root, + 'config', + 'core.hooksPath', + join(root, '.no-such-hooks'), + ]); +} + +function commitAll(root: string): string { + execFileSync('git', ['-C', root, 'add', '.']); + execFileSync('git', ['-C', root, 'commit', '-qm', 'snapshot']); + return execFileSync('git', ['-C', root, 'rev-parse', 'HEAD'], { + encoding: 'utf8', + }).trim(); +} + +function context(provider = 'fake-provider') { + return { + version: 1 as const, + provider, + label: 'Fake project', + domains: ['runtime'], + relatedPaths: ['src/related.ts'], + recommendedTests: ['test:runtime'], + requiredConfigurations: ['debug'], + requiredAgents: ['test-matrix' as const], + unverifiedDimensions: ['Alternate runtime was not exercised'], + verificationNotes: ['Use the repository native test runner'], + }; +} + +function planAt(root: string, plan: object): string { + const path = join(root, 'plan.json'); + write(path, `${JSON.stringify(plan)}\n`); + return path; +} + +function writeManifest(worktree: string, paths = ['src/**']): void { + write( + join(worktree, '.qwen', 'review-context.json'), + JSON.stringify({ + version: 1, + label: 'Manifest project', + rules: [{ paths, domains: ['runtime'] }], + }), + ); +} + +function manifestContext() { + return { + version: 1, + provider: 'manifest', + label: 'Manifest project', + domains: ['runtime'], + relatedPaths: [], + recommendedTests: [], + requiredConfigurations: [], + requiredAgents: [], + unverifiedDimensions: [], + verificationNotes: [], + }; +} + +function run( + root: string, + worktree: string, + plan: object, + providers?: readonly RepositoryContextProvider[], +): { planPath: string; outPath: string } { + const planPath = planAt(root, plan); + const outPath = join(root, 'context.json'); + if (providers === undefined) { + runRepoContext({ plan: planPath, worktree, out: outPath }); + } else { + runRepoContext({ plan: planPath, worktree, out: outPath }, providers); + } + return { planPath, outPath }; +} + +describe('repo-context providers and trust boundary', () => { + it('writes null and clears stale context when no provider matches', () => { + const root = temp(); + const worktree = join(root, 'worktree'); + mkdirSync(worktree); + const { planPath, outPath } = run( + root, + worktree, + { + files: [{ path: 'src/change.ts' }], + repositoryContext: context(), + }, + [], + ); + expect(readJson(outPath)).toBeNull(); + expect(readJson(planPath)).not.toHaveProperty('repositoryContext'); + }); + + it('passes sorted unique changed paths and local identity to a provider', () => { + const root = temp(); + const worktree = join(root, 'worktree'); + write(join(worktree, '.review', 'identity'), 'local\n'); + const provide = vi.fn((input) => { + expect(input.worktree).toBe(realpathSync(worktree)); + expect(input.changedPaths).toEqual(['src/a.ts', 'src/b.ts']); + expect(input.readIdentityFile('.review/identity')).toBe('local'); + expect(input.readIdentityFile('.review/missing')).toBeNull(); + return context(); + }); + const { planPath, outPath } = run( + root, + worktree, + { + files: [ + { path: 'src/b.ts' }, + { path: 'src/a.ts' }, + { path: 'src/a.ts' }, + ], + }, + [{ provide }], + ); + expect(provide).toHaveBeenCalledOnce(); + expect(readJson(outPath)).toEqual(context()); + expect(readJson(planPath)).toHaveProperty('repositoryContext', context()); + }); + + it('uses the trusted base manifest for pull-request opt in and opt out', () => { + const root = temp(); + const worktree = join(root, 'repository'); + initGit(worktree); + writeManifest(worktree); + write(join(worktree, 'src', 'change.ts'), 'base\n'); + const base = commitAll(worktree); + // A second commit makes HEAD != mergeBaseSha: a reader that took the + // manifest from HEAD (instead of the recorded base) would see the + // forged non-matching scope and drop the context. + writeManifest(worktree, ['docs/**']); + commitAll(worktree); + expect( + readJson( + run(join(root, 'base-enabled'), worktree, { + files: [{ path: 'src/change.ts' }], + mergeBaseSha: base, + }).outPath, + ), + ).toEqual(manifestContext()); + + const second = join(root, 'second'); + initGit(second); + writeManifest(second, ['docs/**']); + write(join(second, 'src', 'change.ts'), 'base\n'); + const disabledBase = commitAll(second); + // Head-side commit carries a matching manifest that must never opt IN. + writeManifest(second); + commitAll(second); + expect( + readJson( + run(join(root, 'base-disabled'), second, { + files: [{ path: 'src/change.ts' }], + mergeBaseSha: disabledBase, + }).outPath, + ), + ).toBeNull(); + }); + + it('reads pull-request identity only from the trusted base commit', () => { + const root = temp(); + const worktree = join(root, 'repository'); + initGit(worktree); + write(join(worktree, '.review', 'identity'), 'base\n'); + write(join(worktree, 'src', 'change.ts'), 'base\n'); + const base = commitAll(worktree); + // Commit the forged head-side identity so HEAD != mergeBaseSha: a reader + // keyed on HEAD would return 'head' and fail the assertion. + write(join(worktree, '.review', 'identity'), 'head\n'); + commitAll(worktree); + const provide = vi.fn((input) => { + expect(input.readIdentityFile('.review/identity')).toBe('base'); + return context(); + }); + const { outPath } = run( + root, + worktree, + { + files: [{ path: 'src/change.ts' }], + mergeBaseSha: base, + }, + [{ provide }], + ); + // The provider MUST have run: an early return or a swallowed provider + // error would otherwise keep every inner expect unexecuted and green. + expect(provide).toHaveBeenCalledOnce(); + expect(readJson(outPath)).toEqual(context()); + }); + + it('does not let the current tree opt in or opt out of base identity', () => { + const root = temp(); + const worktree = join(root, 'repository'); + initGit(worktree); + write(join(worktree, '.review', 'identity'), 'enabled\n'); + write(join(worktree, 'src', 'change.ts'), 'base\n'); + const base = commitAll(worktree); + write(join(worktree, '.review', 'identity'), 'disabled\n'); + commitAll(worktree); + const enabled: RepositoryContextProvider = { + provide(input) { + return input.readIdentityFile('.review/identity') === 'enabled' + ? context() + : null; + }, + }; + expect( + readJson( + run( + join(root, 'base-enabled'), + worktree, + { files: [{ path: 'src/change.ts' }], mergeBaseSha: base }, + [enabled], + ).outPath, + ), + ).toEqual(context()); + + const second = join(root, 'second'); + initGit(second); + write(join(second, '.review', 'identity'), 'disabled\n'); + write(join(second, 'src', 'change.ts'), 'base\n'); + const disabledBase = commitAll(second); + write(join(second, '.review', 'identity'), 'enabled\n'); + commitAll(second); + expect( + readJson( + run( + join(root, 'base-disabled'), + second, + { + files: [{ path: 'src/change.ts' }], + mergeBaseSha: disabledBase, + }, + [enabled], + ).outPath, + ), + ).toBeNull(); + }); + + it('writes null when the identity is absent at the base commit', () => { + // "Absent at the base" is a definite state (`ls-tree` exits 0 with no + // output), distinct from "git failed" — and a head-side manifest never + // substitutes for it: deleting the existence probe would make `git show` + // throw and this test fail instead of returning null. + const root = temp(); + const worktree = join(root, 'repository'); + initGit(worktree); + write(join(worktree, 'src', 'change.ts'), 'base\n'); + const base = commitAll(worktree); + // Commit the head-side manifest: HEAD != mergeBaseSha, so a probe keyed + // on HEAD would find the manifest and fail closed on the base read. + writeManifest(worktree); + commitAll(worktree); + const { planPath, outPath } = run(root, worktree, { + files: [{ path: 'src/change.ts' }], + mergeBaseSha: base, + }); + expect(readJson(outPath)).toBeNull(); + expect(readJson(planPath)).not.toHaveProperty('repositoryContext'); + }); + + // Committed symlink objects are not portable to Windows runners. + it.skipIf(process.platform === 'win32')( + 'follows a committed symlink identity like the worktree reader', + () => { + const root = temp(); + const worktree = join(root, 'repository'); + initGit(worktree); + write( + join(worktree, '.qwen', 'manifest.json'), + JSON.stringify({ + version: 1, + label: 'Manifest project', + rules: [{ paths: ['src/**'], domains: ['runtime'] }], + }), + ); + symlinkSync( + 'manifest.json', + join(worktree, '.qwen', 'review-context.json'), + ); + write(join(worktree, 'src', 'change.ts'), 'base\n'); + const base = commitAll(worktree); + + expect( + readJson( + run(join(root, 'pr'), worktree, { + files: [{ path: 'src/change.ts' }], + mergeBaseSha: base, + }).outPath, + ), + ).toEqual(manifestContext()); + expect( + readJson( + run(join(root, 'local'), worktree, { + files: [{ path: 'src/change.ts' }], + }).outPath, + ), + ).toEqual(manifestContext()); + }, + ); + + it.skipIf(process.platform === 'win32')( + 'fails closed when a committed identity symlink escapes the tree', + () => { + const root = temp(); + const worktree = join(root, 'repository'); + initGit(worktree); + write(join(worktree, 'outside.json'), '{}'); + mkdirSync(join(worktree, '.qwen'), { recursive: true }); + symlinkSync( + '../../outside.json', + join(worktree, '.qwen', 'review-context.json'), + ); + write(join(worktree, 'src', 'change.ts'), 'base\n'); + const base = commitAll(worktree); + expect(() => + run(join(root, 'escape'), worktree, { + files: [{ path: 'src/change.ts' }], + mergeBaseSha: base, + }), + ).toThrow('escapes the worktree'); + }, + ); + + it.skipIf(process.platform === 'win32')( + 'fails closed when the base identity symlink chain exceeds the hop ceiling', + () => { + // A committed symlink cycle is the only shape the hop counter guards + // against; without the counter and the final throw it loops forever. + const root = temp(); + const worktree = join(root, 'repository'); + initGit(worktree); + mkdirSync(join(worktree, '.qwen'), { recursive: true }); + symlinkSync( + 'loop-b.json', + join(worktree, '.qwen', 'review-context.json'), + ); + symlinkSync( + 'review-context.json', + join(worktree, '.qwen', 'loop-b.json'), + ); + write(join(worktree, 'src', 'change.ts'), 'base\n'); + const base = commitAll(worktree); + expect(() => + run(join(root, 'cycle'), worktree, { + files: [{ path: 'src/change.ts' }], + mergeBaseSha: base, + }), + ).toThrow('identity symlink chain is too deep'); + }, + ); + + it('degrades to a null artifact when the base fetch failed', () => { + // merge-base documents the state as not fatal, fetch-pr warns and + // continues, base-tree refuses the possibly stale sha — repo-context + // degrades like the unresolved base instead of halting the review. + const root = temp(); + const worktree = join(root, 'repository'); + initGit(worktree); + writeManifest(worktree); + write(join(worktree, 'src', 'change.ts'), 'base\n'); + const base = commitAll(worktree); + const provide = vi.fn(() => + context(), + ); + const { planPath, outPath } = run( + root, + worktree, + { + files: [{ path: 'src/change.ts' }], + mergeBaseSha: base, + baseFetchFailed: true, + }, + [{ provide }], + ); + expect(provide).not.toHaveBeenCalled(); + expect(readJson(outPath)).toBeNull(); + expect(readJson(planPath)).not.toHaveProperty('repositoryContext'); + }); + + it('fails closed for an invalid or unresolvable base', () => { + const root = temp(); + const worktree = join(root, 'repository'); + initGit(worktree); + write(join(worktree, 'src', 'change.ts'), 'base\n'); + commitAll(worktree); + const provider: RepositoryContextProvider = { provide: () => context() }; + expect(() => + run( + join(root, 'invalid'), + worktree, + { files: [{ path: 'src/change.ts' }], mergeBaseSha: 'nope' }, + [provider], + ), + ).toThrow('mergeBaseSha is invalid'); + expect(() => + run( + join(root, 'stale'), + worktree, + { + files: [{ path: 'src/change.ts' }], + mergeBaseSha: '0'.repeat(40), + }, + [provider], + ), + ).toThrow('cannot be resolved'); + }); + + it('writes null without consulting the worktree when the base never resolved', () => { + // fetch-pr records `mergeBaseSha: null` and degrades rather than failing + // the review; repo-context must degrade the same way — and MUST NOT fall + // back to the worktree reader, which would take the manifest from the PR + // head, the exact read the trust boundary forbids. + const root = temp(); + const worktree = join(root, 'repository'); + initGit(worktree); + writeManifest(worktree); // a head-side manifest that must never be read + write(join(worktree, 'src', 'change.ts'), 'base\n'); + commitAll(worktree); + const provide = vi.fn(() => + context(), + ); + + const first = run( + join(root, 'null-base'), + worktree, + { files: [{ path: 'src/change.ts' }], mergeBaseSha: null }, + [{ provide }], + ); + expect(provide).not.toHaveBeenCalled(); + expect(readJson(first.outPath)).toBeNull(); + expect(readJson(first.planPath)).not.toHaveProperty('repositoryContext'); + + // A failed base fetch with no resolved sha degrades the same way: there + // is nothing stale, and nothing to trust. + const second = run( + join(root, 'null-base-fetch-failed'), + worktree, + { + files: [{ path: 'src/change.ts' }], + mergeBaseSha: null, + baseFetchFailed: true, + }, + [{ provide }], + ); + expect(provide).not.toHaveBeenCalled(); + expect(readJson(second.outPath)).toBeNull(); + }); + + it('normalizes identity content identically in local and pull-request modes', () => { + // A provider that exact-compares a marker file must get the same value in + // both modes: CRLF normalised to LF, surrounding whitespace trimmed. The + // interior CRLF is the pin — a single trailing one is stripped by trim + // alone. + const root = temp(); + const worktree = join(root, 'worktree'); + write(join(worktree, '.review', 'identity'), 'token\r\ntail\r\n'); + const localProvide = vi.fn( + (input) => { + expect(input.readIdentityFile('.review/identity')).toBe('token\ntail'); + return context(); + }, + ); + run(root, worktree, { files: [{ path: 'src/change.ts' }] }, [ + { provide: localProvide }, + ]); + expect(localProvide).toHaveBeenCalledOnce(); + + const repository = join(root, 'repository'); + initGit(repository); + execFileSync('git', ['-C', repository, 'config', 'core.autocrlf', 'false']); + write(join(repository, '.review', 'identity'), 'token\r\ntail\r\n'); + write(join(repository, 'src', 'change.ts'), 'base\n'); + const base = commitAll(repository); + const prProvide = vi.fn((input) => { + expect(input.readIdentityFile('.review/identity')).toBe('token\ntail'); + return context(); + }); + run( + root, + repository, + { files: [{ path: 'src/change.ts' }], mergeBaseSha: base }, + [{ provide: prProvide }], + ); + expect(prProvide).toHaveBeenCalledOnce(); + }); + + it('treats an identity path resolving to the worktree root as absent', () => { + const root = temp(); + const worktree = join(root, 'worktree'); + mkdirSync(worktree); + symlinkSync(worktree, join(worktree, 'root-link')); + const provide = vi.fn((input) => { + expect(input.readIdentityFile('root-link')).toBeNull(); + return context(); + }); + run(root, worktree, { files: [{ path: 'src/change.ts' }] }, [{ provide }]); + expect(provide).toHaveBeenCalledOnce(); + }); + + // Permission-based failure injection is meaningless to root, and chmod(0) + // does not block reads on Windows — the repo convention for this case. + const isRoot = process.platform === 'win32' || process.getuid?.() === 0; + + it.skipIf(isRoot)( + 'fails closed when a present local identity file cannot be read', + () => { + const root = temp(); + const worktree = join(root, 'worktree'); + const identity = join(worktree, '.review', 'identity'); + write(identity, 'token\n'); + chmodSync(identity, 0); + try { + expect(() => + run(root, worktree, { files: [{ path: 'src/change.ts' }] }, [ + { + provide(input) { + input.readIdentityFile('.review/identity'); + return context(); + }, + }, + ]), + ).toThrow(); + } finally { + chmodSync(identity, 0o644); + } + }, + ); + + it.skipIf(isRoot)('keeps the artifact when the plan write fails', () => { + // Write ordering is artifact-then-plan on purpose: when the plan write is + // the one that fails, the artifact has landed but the plan is untouched, + // so the two never disagree about a run — the next invocation rewrites + // both. + const root = temp(); + const worktree = join(root, 'worktree'); + mkdirSync(worktree); + const planDir = join(root, 'plan-dir'); + mkdirSync(planDir); + const planPath = join(planDir, 'plan.json'); + write( + planPath, + `${JSON.stringify({ files: [{ path: 'src/change.ts' }] })}\n`, + ); + const outPath = join(root, 'context.json'); + const before = readFileSync(planPath, 'utf8'); + chmodSync(planDir, 0o555); + try { + expect(() => + runRepoContext({ plan: planPath, worktree, out: outPath }, [ + { provide: () => context() }, + ]), + ).toThrow(); + expect(readJson(outPath)).toEqual(context()); + expect(readFileSync(planPath, 'utf8')).toBe(before); + } finally { + chmodSync(planDir, 0o755); + } + }); + + it('supports recorded linked-worktree paths', () => { + const root = temp(); + const repository = join(root, 'repository'); + initGit(repository); + write(join(repository, 'src', 'change.ts'), 'base\n'); + commitAll(repository); + const linked = join(root, 'linked'); + execFileSync('git', ['-C', repository, 'worktree', 'add', '-q', linked]); + const provide = vi.fn((input) => { + // The provider must receive the LINKED worktree, not the main + // repository root the plan's --git-common-dir resolves to. + expect(input.worktree).toBe(realpathSync(linked)); + return context(); + }); + const { planPath, outPath } = run( + root, + linked, + { + files: [{ path: 'src/change.ts' }], + worktreePath: '../linked', + }, + [{ provide }], + ); + expect(provide).toHaveBeenCalledOnce(); + expect(readJson(outPath)).toEqual(context()); + expect(readJson(planPath)).toHaveProperty('repositoryContext', context()); + }); + + it('rejects a recorded worktree path that matches no checkout', () => { + // The guard's rejection branch: a plan recorded for one checkout must + // not be served identity reads from a different worktree. + const root = temp(); + const worktree = join(root, 'worktree'); + mkdirSync(worktree); + expect(() => + run(root, worktree, { + files: [{ path: 'src/change.ts' }], + worktreePath: '../somewhere-else', + }), + ).toThrow('does not match plan.worktreePath'); + }); + + it('degrades to a null artifact when the base identity path is a directory', () => { + // `git show :` exits 0 with the tree listing; the guard must + // map a directory at the identity path to null — worktree mode's + // `isFile() === false` — instead of feeding the listing to the parser, + // whose throw would fail the whole review closed over a clean degrade. + const root = temp(); + const worktree = join(root, 'repository'); + initGit(worktree); + write(join(worktree, '.qwen', 'review-context.json', 'inner.txt'), '{}\n'); + write(join(worktree, 'src', 'change.ts'), 'base\n'); + const base = commitAll(worktree); + const { outPath } = run(root, worktree, { + files: [{ path: 'src/change.ts' }], + mergeBaseSha: base, + }); + expect(readJson(outPath)).toBeNull(); + }); + + it.skipIf(process.platform === 'win32')( + 'reads a broken trailing-slash identity symlink as absent in both modes', + () => { + // A target ending in `/` that resolves to a regular file fails + // ENOTDIR on disk (local mode); base mode must degrade the same way + // instead of dropping the empty segment and returning content. + const root = temp(); + const worktree = join(root, 'repository'); + initGit(worktree); + write( + join(worktree, '.qwen', 'manifest.json'), + JSON.stringify({ + version: 1, + label: 'Manifest project', + rules: [{ paths: ['src/**'], domains: ['runtime'] }], + }), + ); + symlinkSync( + 'manifest.json/', + join(worktree, '.qwen', 'review-context.json'), + ); + write(join(worktree, 'src', 'change.ts'), 'base\n'); + const base = commitAll(worktree); + + const pr = run(join(root, 'pr'), worktree, { + files: [{ path: 'src/change.ts' }], + mergeBaseSha: base, + }); + expect(readJson(pr.outPath)).toBeNull(); + const local = run(join(root, 'local'), worktree, { + files: [{ path: 'src/change.ts' }], + }); + expect(readJson(local.outPath)).toBeNull(); + }, + ); + + it('rejects identity traversal and symlink escapes', () => { + const root = temp(); + const worktree = join(root, 'worktree'); + mkdirSync(worktree); + write(join(root, 'outside'), 'secret\n'); + symlinkSync(join(root, 'outside'), join(worktree, 'identity')); + for (const identityPath of ['../outside', '/outside', 'identity']) { + expect(() => + run( + join(root, identityPath.replaceAll('/', '_')), + worktree, + { files: [{ path: 'src/change.ts' }] }, + [ + { + provide(input) { + input.readIdentityFile(identityPath); + return context(); + }, + }, + ], + ), + ).toThrow(); + } + }); + + it('skips unsafe changed paths instead of aborting the step', () => { + // Changed paths are only matched against manifest globs, never opened, so + // an unsafe-but-real path (a backslash is a legal POSIX filename byte) + // must not kill a step that runs on every review — it just cannot match. + const root = temp(); + const worktree = join(root, 'worktree'); + mkdirSync(worktree); + const provide = vi.fn((input) => { + expect(input.changedPaths).toEqual(['src/ok.ts']); + return context(); + }); + run( + root, + worktree, + { files: [{ path: '../secret' }, { path: 'src/ok.ts' }] }, + [{ provide }], + ); + expect(provide).toHaveBeenCalledOnce(); + }); + + it('still rejects a corrupted plan whose file paths are not strings', () => { + const root = temp(); + const worktree = join(root, 'worktree'); + mkdirSync(worktree); + expect(() => run(root, worktree, { files: [{ path: 42 }] })).toThrow( + 'plan.files[0].path is invalid', + ); + }); + + it('rejects plan/out aliases and preserves the plan on artifact failure', () => { + const root = temp(); + const worktree = join(root, 'worktree'); + mkdirSync(worktree); + const planPath = planAt(root, { files: [{ path: 'src/change.ts' }] }); + expect(() => + runRepoContext({ plan: planPath, worktree, out: planPath }, [ + { provide: () => context() }, + ]), + ).toThrow('--out must differ'); + + const alias = join(root, 'alias.json'); + linkSync(planPath, alias); + expect(() => + runRepoContext({ plan: planPath, worktree, out: alias }, [ + { provide: () => context() }, + ]), + ).toThrow('--out must differ'); + + const before = readFileSync(planPath, 'utf8'); + const outDirectory = join(root, 'out-directory'); + mkdirSync(outDirectory); + expect(() => + runRepoContext({ plan: planPath, worktree, out: outDirectory }, [ + { provide: () => context() }, + ]), + ).toThrow(); + expect(readFileSync(planPath, 'utf8')).toBe(before); + }); + + it('rejects invalid provider output', () => { + const root = temp(); + const worktree = join(root, 'worktree'); + mkdirSync(worktree); + expect(() => + run(root, worktree, { files: [{ path: 'src/change.ts' }] }, [ + { + provide: () => + ({ + ...context(), + requiredAgents: ['unknown-role'], + }) as never, + }, + ]), + ).toThrow('unsupported role'); + }); + + it('declares all required command options and uses the default manifest provider', () => { + const option = vi.fn().mockReturnThis(); + const built = (repoContextCommand.builder as (yargs: unknown) => unknown)({ + option, + }); + expect(built).toBeDefined(); + // Names AND the `demandOption` flags — the flags are what makes the + // options required; without them the handler dies on a raw TypeError + // instead of yargs' clean missing-argument usage error. + expect( + option.mock.calls.map(([name, config]) => [ + name, + (config as { demandOption?: boolean }).demandOption, + ]), + ).toEqual([ + ['plan', true], + ['worktree', true], + ['out', true], + ]); + + const root = temp(); + const worktree = join(root, 'worktree'); + writeManifest(worktree); + const plan = planAt(root, { files: [{ path: 'src/change.ts' }] }); + const out = join(root, 'context.json'); + (repoContextCommand.handler as (args: unknown) => void)({ + plan, + worktree, + out, + }); + expect(readJson(out)).toEqual(manifestContext()); + }); + + it('preserves plan fields the command does not know across the rewrite', () => { + // The rewrite must carry every field downstream steps read — `files[]`, + // `effort`, and anything else the plan holds — not just + // `repositoryContext`. + const root = temp(); + const worktree = join(root, 'worktree'); + mkdirSync(worktree); + const { planPath } = run( + root, + worktree, + { + files: [{ path: 'src/change.ts' }], + effort: 'high', + customField: 'kept', + }, + [{ provide: () => context() }], + ); + expect(readJson(planPath)).toMatchObject({ + files: [{ path: 'src/change.ts' }], + effort: 'high', + customField: 'kept', + repositoryContext: context(), + }); + }); + + it('rejects a corrupted plan whose files field is not an array', () => { + // The sibling gate for item-level shape: a corrupted plan must exit + // fail-closed, not produce empty changedPaths and a null artifact. + const root = temp(); + const worktree = join(root, 'worktree'); + mkdirSync(worktree); + expect(() => run(root, worktree, { files: 'oops' })).toThrow( + 'plan.files must be an array', + ); + }); + + it('creates a missing --out parent directory', () => { + const root = temp(); + const worktree = join(root, 'worktree'); + mkdirSync(worktree); + const planPath = planAt(root, { files: [{ path: 'src/change.ts' }] }); + const outPath = join(root, 'fresh-subdir', 'context.json'); + runRepoContext({ plan: planPath, worktree, out: outPath }, [ + { provide: () => context() }, + ]); + expect(readJson(outPath)).toEqual(context()); + }); + + it('fails closed when the local identity exceeds the size limit', () => { + // The identity read is size-capped BEFORE its content is parsed; an + // oversized manifest must throw rather than masquerade as readable. + const root = temp(); + const worktree = join(root, 'worktree'); + write( + join(worktree, '.qwen', 'review-context.json'), + `"${'x'.repeat(MAX_IDENTITY_BYTES)}"`, + ); + expect(() => + run(root, worktree, { files: [{ path: 'src/change.ts' }] }), + ).toThrow('exceeds the size limit'); + }); + + // Windows symlink creation needs elevated privileges. + it.skipIf(process.platform === 'win32')( + 'canonicalizes a symlinked --worktree argument', + () => { + // Local-mode containment compares fully realpathed identity reads + // against the canonicalised argument; with an un-canonicalised + // argument whose ancestor is a symlink (macOS /tmp, linked mounts or + // home dirs) every identity read computes an escaping path and the + // whole review fails closed. + const root = temp(); + const worktree = join(root, 'worktree'); + write(join(worktree, '.review', 'identity'), 'local\n'); + const link = join(root, 'worktree-link'); + symlinkSync(worktree, link); + const provide = vi.fn((input) => { + expect(input.worktree).toBe(realpathSync(worktree)); + expect(input.readIdentityFile('.review/identity')).toBe('local'); + return context(); + }); + run(root, link, { files: [{ path: 'src/change.ts' }] }, [{ provide }]); + expect(provide).toHaveBeenCalledOnce(); + }, + ); + + it('attaches context from a SHA-256 repository', () => { + // The mergeBaseSha validator's 64-hex alternative exists for SHA-256 + // repositories; without this pin a future simplification to 40-hex + // ships green and hard-fails every PR review in such a repository on a + // correctly recorded value. + const root = temp(); + const worktree = join(root, 'repository'); + try { + execFileSync('git', [ + 'init', + '-q', + '--template=', + '--object-format=sha256', + worktree, + ]); + } catch { + return; // git < 2.29 has no object format + } + execFileSync('git', [ + '-C', + worktree, + 'config', + 'user.email', + 'test@example.com', + ]); + execFileSync('git', ['-C', worktree, 'config', 'user.name', 'Test']); + execFileSync('git', ['-C', worktree, 'config', 'commit.gpgsign', 'false']); + writeManifest(worktree); + write(join(worktree, 'src', 'change.ts'), 'base\n'); + const base = commitAll(worktree); + expect(base).toMatch(/^[0-9a-f]{64}$/); + expect( + readJson( + run(join(root, 'sha256'), worktree, { + files: [{ path: 'src/change.ts' }], + mergeBaseSha: base, + }).outPath, + ), + ).toEqual(manifestContext()); + }); + + it.skipIf(process.platform === 'win32')( + 'reads a `.`-targeted identity symlink as absent in both modes', + () => { + // A target like `a/.` walks THROUGH `a`, so the kernel fails ENOTDIR + // when `a` is a file; base mode must degrade to null exactly like the + // worktree reader instead of dropping the `.` and reading the blob — + // the "base mode reads strictly less, never more" invariant. + const root = temp(); + const worktree = join(root, 'repository'); + initGit(worktree); + write(join(worktree, '.qwen', 'a'), '{}'); + symlinkSync('a/.', join(worktree, '.qwen', 'review-context.json')); + write(join(worktree, 'src', 'change.ts'), 'base\n'); + const base = commitAll(worktree); + const pr = run(join(root, 'pr'), worktree, { + files: [{ path: 'src/change.ts' }], + mergeBaseSha: base, + }); + expect(readJson(pr.outPath)).toBeNull(); + const local = run(join(root, 'local'), worktree, { + files: [{ path: 'src/change.ts' }], + }); + expect(readJson(local.outPath)).toBeNull(); + }, + ); +}); diff --git a/packages/cli/src/commands/review/repo-context.ts b/packages/cli/src/commands/review/repo-context.ts new file mode 100644 index 00000000000..8b5b5a9b0a8 --- /dev/null +++ b/packages/cli/src/commands/review/repo-context.ts @@ -0,0 +1,434 @@ +/** + * @license + * Copyright 2026 Qwen Team + * SPDX-License-Identifier: Apache-2.0 + */ + +import type { CommandModule } from 'yargs'; +import { atomicWriteFileSync } from '@qwen-code/qwen-code-core'; +import { + existsSync, + mkdirSync, + readFileSync, + realpathSync, + statSync, +} from 'node:fs'; +import { dirname, isAbsolute, relative, resolve, sep } from 'node:path'; +import { writeStdoutLine } from '../../utils/stdioHelpers.js'; +import { git, gitOpt, gitRaw } from './lib/git.js'; +import { manifestRepositoryContextProvider } from './lib/manifest-repository-context.js'; +import { + isSafeRepositoryRelativePath, + MAX_IDENTITY_BYTES, + type RepositoryContext, + type RepositoryContextProvider, + validateRepositoryContext, +} from './lib/repository-context.js'; +import { stringifyPlanReport } from './lib/report.js'; + +interface RepoContextArgs { + plan: string; + worktree: string; + out: string; +} + +interface PlanFile { + path: unknown; +} + +interface MutablePlan { + files?: unknown; + worktreePath?: unknown; + mergeBaseSha?: unknown; + baseFetchFailed?: unknown; + repositoryContext?: unknown; + [key: string]: unknown; +} + +export const REPOSITORY_CONTEXT_PROVIDERS: readonly RepositoryContextProvider[] = + [manifestRepositoryContextProvider]; + +function sameFile(left: string, right: string): boolean { + if (left === right) return true; + if (!existsSync(left) || !existsSync(right)) return false; + const leftStat = statSync(left); + const rightStat = statSync(right); + return leftStat.dev === rightStat.dev && leftStat.ino === rightStat.ino; +} + +function recordedWorktreeMatches( + recordedPath: string, + worktree: string, +): boolean { + const candidates = [resolve(recordedPath)]; + if (!isAbsolute(recordedPath)) { + const commonDir = gitOpt('-C', worktree, 'rev-parse', '--git-common-dir'); + if (commonDir !== null) { + candidates.push( + resolve(dirname(resolve(worktree, commonDir)), recordedPath), + ); + } + } + return candidates.some( + (candidate) => + existsSync(candidate) && realpathSync(candidate) === worktree, + ); +} + +/** + * Which identity source this plan may read from. `local` — no merge base was + * ever recorded (a capture-local / plan-diff plan): the worktree. `base` — a + * trusted, resolved merge base: that commit only. `none` — NO source, for + * two PR states the pipeline itself degrades: a base that never resolved + * (`fetch-pr` records `mergeBaseSha: null` rather than failing) and a FAILED + * base fetch (`merge-base` documents the state as not fatal, `fetch-pr` + * warns and continues on a possibly stale sha, `base-tree` refuses one — a + * possibly stale sha is not a trusted identity source either). The shape + * matters: the natural-looking fallback would read the manifest from the PR + * head, the exact read the trust boundary exists to forbid, so the command + * writes a `null` artifact instead — the same degradation `fetch-pr` chose, + * taken one step. + */ +type MergeBaseResolution = + | { kind: 'local' } + | { kind: 'base'; sha: string } + | { kind: 'none' }; + +function trustedMergeBase( + plan: MutablePlan, + worktree: string, +): MergeBaseResolution { + if (plan.mergeBaseSha === undefined) return { kind: 'local' }; + if (plan.baseFetchFailed === true || plan.mergeBaseSha === null) { + return { kind: 'none' }; + } + if ( + typeof plan.mergeBaseSha !== 'string' || + !/^(?:[0-9a-f]{40}|[0-9a-f]{64})$/i.test(plan.mergeBaseSha) + ) { + throw new Error('repo-context: plan.mergeBaseSha is invalid'); + } + if ( + gitOpt( + '-C', + worktree, + 'cat-file', + '-e', + `${plan.mergeBaseSha}^{commit}`, + ) === null + ) { + throw new Error('repo-context: plan.mergeBaseSha cannot be resolved'); + } + return { kind: 'base', sha: plan.mergeBaseSha }; +} + +// `git` normalises stdout (CRLF to LF, trimmed); the worktree read must return +// the same shape, or a provider that exact-compares an identity file gets one +// value in a PR review and another in a local review of the same repository. +function normalizeIdentityContent(content: string): string { + return content.replace(/\r\n/g, '\n').trim(); +} + +function isAbsentError(error: unknown): boolean { + const code = (error as NodeJS.ErrnoException | null)?.code; + return code === 'ENOENT' || code === 'ENOTDIR'; +} + +/** + * One `git ls-tree -- ` entry. Exit 0 with empty output is git's + * DEFINITE "absent at this revision" — unlike `cat-file -e`, whose non-zero + * exit cannot be told from a failed git call, so a throwing git call below + * stays a throw (fail closed) and never masquerades as "not this repository". + */ +function baseTreeEntry( + worktree: string, + mergeBase: string, + path: string, +): { mode: string; type: string } | null { + const output = git('-C', worktree, 'ls-tree', mergeBase, '--', path); + if (output === '') return null; + const [mode, type] = output.split(/\s+/); + return { mode, type }; +} + +function readBaseBlob(worktree: string, mergeBase: string, path: string) { + try { + // `gitRaw`'s raised maxBuffer: `git()` inherits execFileSync's 1 MB + // default, so a schema-legal manifest past 1 MB would die with ENOBUFS + // in PR mode while the worktree branch reads it without a cap. + return gitRaw('-C', worktree, 'show', `${mergeBase}:${path}`).toString( + 'utf8', + ); + } catch (error) { + throw new Error( + `repo-context: identity read failed for ${path}: ` + + `${(error as Error).message}`, + ); + } +} + +/** + * Resolve a committed symlink's target the way the filesystem resolves one: + * relative to the link's own directory. `null` means the target escapes the + * tree (absolute, or climbs past the root); `''` means it climbs to the tree + * root itself — a directory, never an identity file. + */ +function resolveTreeSymlinkTarget( + fromPath: string, + target: string, +): string | null { + if (target.startsWith('/') || /^[A-Za-z]:/.test(target)) return null; + const segments = `${dirname(fromPath)}/${target}`.split('/'); + const resolved: string[] = []; + for (const segment of segments) { + if (segment === '' || segment === '.') continue; + if (segment === '..') { + if (resolved.length === 0) return null; + resolved.pop(); + continue; + } + resolved.push(segment); + } + return resolved.join('/'); +} + +const MAX_IDENTITY_SYMLINK_HOPS = 16; + +/** + * The base-mode identity read, mirroring the worktree branch where git can: + * `ls-tree` mode stands in for `lstat`/`statSync` (`cat-file -e` would + * happily "exist" for a tree or symlink entry and hand a provider content + * the worktree branch can never produce), committed symlinks are followed + * under the same containment rule `realpathSync` enforces on disk, and a + * directory yields `null` exactly like `isFile() === false`. One known + * divergence: `ls-tree` never descends through a symlinked intermediate + * path COMPONENT, so an identity below one reads `null` here while the + * worktree branch follows it. The direction is fail-safe — base mode reads + * strictly less, never more — so the gap degrades to "no context", not a + * trust hole. + */ +function readBaseIdentity( + worktree: string, + mergeBase: string, + relativePath: string, +): string | null { + let path = relativePath; + // A symlink target ending in `/`, `.`, or `..` requires the finally + // resolved entry to be a directory, exactly the way realpathSync fails + // ENOTDIR on disk — without this the two modes diverge on a broken + // trailing-component link (a target like `a/.` walks THROUGH `a`). + let requireDirectory = false; + for (let hop = 0; hop < MAX_IDENTITY_SYMLINK_HOPS; hop++) { + const entry = baseTreeEntry(worktree, mergeBase, path); + if (entry === null) return null; + if (entry.mode === '120000') { + const target = readBaseBlob(worktree, mergeBase, path); + const lastSegment = target.split('/').pop(); + if (target.endsWith('/') || lastSegment === '.' || lastSegment === '..') { + requireDirectory = true; + } + const resolved = resolveTreeSymlinkTarget(path, target); + if (resolved === null) { + throw new Error( + `repo-context: identity path escapes the worktree: ` + + `${JSON.stringify(relativePath)}`, + ); + } + if (resolved === '') return null; + path = resolved; + continue; + } + if (entry.type !== 'blob') return null; + if (requireDirectory) return null; + return normalizeIdentityContent(readBaseBlob(worktree, mergeBase, path)); + } + throw new Error( + `repo-context: identity symlink chain is too deep: ` + + `${JSON.stringify(relativePath)}`, + ); +} + +function identityReader( + worktree: string, + mergeBase: string | null, +): (relativePath: string) => string | null { + return (relativePath) => { + if (!isSafeRepositoryRelativePath(relativePath)) { + throw new Error( + `repo-context: identity path is unsafe: ${JSON.stringify(relativePath)}`, + ); + } + if (mergeBase !== null) { + return readBaseIdentity(worktree, mergeBase, relativePath); + } + const candidate = resolve(worktree, relativePath); + let resolved: string; + try { + resolved = realpathSync(candidate); + } catch (error) { + if (isAbsentError(error)) return null; + throw error; + } + const contained = relative(worktree, resolved); + // A path resolving to the worktree root itself is a directory, never an + // identity file; it falls out at the isFile check, not here. + if ( + isAbsolute(contained) || + contained === '..' || + contained.startsWith(`..${sep}`) + ) { + throw new Error( + `repo-context: identity path escapes the worktree: ${JSON.stringify(relativePath)}`, + ); + } + try { + const stat = statSync(resolved); + if (!stat.isFile()) return null; + // Fail closed before reading (and therefore parsing) an oversized + // identity — the manifest provider's threat model is an + // attacker-committed file, and JSON.parse runs before any schema + // validation can reject it. + if (stat.size > MAX_IDENTITY_BYTES) { + throw new Error( + `repo-context: identity read exceeds the size limit: ` + + `${JSON.stringify(relativePath)}`, + ); + } + return normalizeIdentityContent(readFileSync(resolved, 'utf8')); + } catch (error) { + if (isAbsentError(error)) return null; + throw error; + } + }; +} + +function readPlan(path: string): MutablePlan { + let value: unknown; + try { + value = JSON.parse(readFileSync(path, 'utf8')); + } catch (error) { + throw new Error(`Cannot read plan ${path}: ${(error as Error).message}`); + } + if (typeof value !== 'object' || value === null || Array.isArray(value)) { + throw new Error('repo-context: plan must be a JSON object'); + } + return value as MutablePlan; +} + +function changedPaths(plan: MutablePlan): string[] { + if (!Array.isArray(plan.files)) { + throw new Error('repo-context: plan.files must be an array'); + } + const paths: string[] = []; + for (const [index, file] of plan.files.entries()) { + const path = + typeof file === 'object' && file !== null + ? (file as PlanFile).path + : undefined; + if (typeof path !== 'string') { + throw new Error(`repo-context: plan.files[${index}].path is invalid`); + } + // Changed paths are only ever MATCHED against manifest globs, never opened, + // so an unsafe-but-real path (a backslash is a legal POSIX filename byte) + // is skipped rather than aborting a step that runs on every review. + if (isSafeRepositoryRelativePath(path)) paths.push(path); + } + return [...new Set(paths)].sort(); +} + +function contextFromProviders( + providers: readonly RepositoryContextProvider[], + worktree: string, + paths: string[], + readIdentityFile: (relativePath: string) => string | null, +): RepositoryContext | null { + for (const provider of providers) { + const context = provider.provide({ + worktree, + changedPaths: paths, + readIdentityFile, + }); + if (context !== null) return validateRepositoryContext(context); + } + return null; +} + +export function runRepoContext( + args: RepoContextArgs, + providers: readonly RepositoryContextProvider[] = REPOSITORY_CONTEXT_PROVIDERS, +): void { + const planPath = resolve(args.plan); + const outPath = resolve(args.out); + if (sameFile(planPath, outPath)) { + throw new Error('repo-context: --out must differ from --plan'); + } + const worktree = realpathSync(resolve(args.worktree)); + if (!statSync(worktree).isDirectory()) { + throw new Error(`repo-context: worktree is not a directory: ${worktree}`); + } + + const plan = readPlan(planPath); + if (plan.worktreePath !== undefined) { + if ( + typeof plan.worktreePath !== 'string' || + plan.worktreePath.length === 0 + ) { + throw new Error('repo-context: plan.worktreePath is invalid'); + } + if (!recordedWorktreeMatches(plan.worktreePath, worktree)) { + throw new Error( + `repo-context: --worktree does not match plan.worktreePath (${worktree} != ${plan.worktreePath})`, + ); + } + } + + const mergeBase = trustedMergeBase(plan, worktree); + const context = + mergeBase.kind === 'none' + ? null + : contextFromProviders( + providers, + worktree, + changedPaths(plan), + identityReader( + worktree, + mergeBase.kind === 'base' ? mergeBase.sha : null, + ), + ); + if (context === null) delete plan.repositoryContext; + else plan.repositoryContext = context; + + mkdirSync(dirname(outPath), { recursive: true }); + atomicWriteFileSync(outPath, `${JSON.stringify(context, null, 2)}\n`); + atomicWriteFileSync(planPath, stringifyPlanReport(plan)); + writeStdoutLine( + context === null + ? `Wrote null repository context to ${outPath}` + : `Wrote repository context (${context.provider}) to ${outPath}`, + ); +} + +export const repoContextCommand: CommandModule = { + command: 'repo-context', + describe: 'Attach bounded repository-specific context to a review plan', + builder: (yargs) => + yargs + .option('plan', { + type: 'string', + demandOption: true, + describe: 'Existing review plan JSON to update', + }) + .option('worktree', { + type: 'string', + demandOption: true, + describe: 'Repository worktree used to resolve context', + }) + .option('out', { + type: 'string', + demandOption: true, + describe: 'Independent repository-context artifact path', + }), + handler: (argv) => { + runRepoContext(argv as unknown as RepoContextArgs); + }, +}; diff --git a/packages/core/src/skills/bundled/review/SKILL.md b/packages/core/src/skills/bundled/review/SKILL.md index 24cf80282f0..941c8a52990 100644 --- a/packages/core/src/skills/bundled/review/SKILL.md +++ b/packages/core/src/skills/bundled/review/SKILL.md @@ -76,7 +76,7 @@ At every effort level, the mechanics of obtaining the diff — worktree flow, di The parser already classified the target, so there is nothing to disambiguate by hand. For a `pr-url` target, determine if the local repo can access this PR: -1. Check if any git remote matches the URL's **host and owner/repo — by exact segment equality, never substring**: run `git remote -v` and parse each remote URL structurally (`git@:/.git` and `https:////(.git)` are the two shapes). A remote matches only when its host equals the verdict's `host` AND its `/` (with any `.git` suffix stripped) equals the verdict's `owner/repo`, both compared case-insensitively as whole segments — `shao/qwen-code` does NOT match a `wenshao/qwen-code` remote, and a `github.com` PR does not match a same-named repo on another host. Substring "contains" matching once allowed exactly those, which is reviewing one repository and posting to another. This still handles forks — a local clone of `wenshao/jdk` with an `upstream` remote pointing to `openjdk/jdk` still matches `openjdk/jdk` PRs exactly. +1. Check if any git remote matches the URL's **host and owner/repo — by exact segment equality, never substring**: run `git remote -v` and parse each remote URL structurally (`git@:/.git` and `https:////(.git)` are the two shapes). A remote matches only when its host equals the verdict's `host` AND its `/` (with any `.git` suffix stripped) equals the verdict's `owner/repo`, both compared case-insensitively as whole segments — `shao/qwen-code` does NOT match a `wenshao/qwen-code` remote, and a `github.com` PR does not match a same-named repo on another host. Substring "contains" matching once allowed exactly those, which is reviewing one repository and posting to another. This still handles forks — a local clone with an `upstream` remote pointing to the target repository matches that repository's PRs exactly. 2. If a matching remote is found, proceed with the **normal worktree flow** — use that remote name (instead of hardcoded `origin`) for `git fetch pull//head:qwen-review/pr-`. In Step 7, use the owner/repo from the URL for posting comments. For a `pr-url` whose `host` is not `github.com` (GitHub Enterprise), **pass `--host ` to every review subcommand that talks to GitHub — `fetch-pr`, `pr-context`, `comment-status`, `presubmit`, and `compose-review`** — which routes all of their `gh` calls via GH_HOST in code; a forgotten host cannot silently retarget them at github.com. The `gh` commands you run directly are still yours to route: prefix Agent 0's `gh pr view`/`gh issue view`, Step 6's residual body fetch, and the Step 7 submission with `GH_HOST= ` (e.g. `GH_HOST=github.example.com gh api ...`). `gh` defaults to `github.com`, so a dropped host makes a call read from and post to the wrong site's `owner/repo`. @@ -127,7 +127,7 @@ Based on the parsed `target.type`: - If SHAs match **and** model matches **but** `--comment` WAS specified → run the full review anyway. Inform the user: "No new code changes. Running review to post inline comments." - If SHAs match **but** model differs → continue. Inform: "Previous review used {cached_model}. Running full review with {{model}} for a second opinion." - - **The setup calls that do not feed each other go out in ONE response — as separate tool calls, never joined with `&&`/`;` into one Shell command** (high and medium effort — at low, Step 2's rules load is skipped and nothing consumes the comment index, so the batch is whatever calls remain). A joined chain changes the failure semantics — a `pr-context` failure must warn-and-continue, not skip the other two — and merges the `warning:` size lines the paging decisions below read. Once `fetch-pr` has returned (and the incremental check, which reads its report, is decided), the next three commands are mutually independent — `pr-context` (below), `comment-status` (below), and Step 2's rules load — every one a read with no side effect the others observe. Issue all three tool calls in a single response, exactly as Step 3 already requires for the agent fan-out, then read their outputs (paging where a file exceeds one read, and those reads can share a response too). The rules load takes `/` — the ref `fetch-pr` just updated; no local-existence probe — **except when the fetch report recorded `baseFetchFailed: true`: drop it from the batch and `git fetch ` first** (on an unresolvable ref `load-rules` reports "no rules found", indistinguishable from a repo that has none, and the review silently enforces nothing). Measured on a real small-PR run: the stretch from `parse-args` to the first agent launch took **7 minutes of wall clock**, one round-trip at a time, on calls that never needed an order. The only orderings that matter: `fetch-pr` before all of them (it creates the worktree and the plan), and `agent-prompt --roster` after the rules load (the roster bakes the rules into every brief). + - **The setup calls that do not feed each other go out in ONE response — as separate tool calls, never joined with `&&`/`;` into one Shell command** (high and medium effort — at low, Step 2's rules load is skipped and nothing consumes the comment index, so the batch is whatever calls remain). A joined chain changes the failure semantics — a `pr-context` failure must warn-and-continue, not skip the other two — and merges the `warning:` size lines the paging decisions below read. Once `fetch-pr` has returned (and the incremental check, which reads its report, is decided), the next three commands are mutually independent — `pr-context` (below), `comment-status` (below), and Step 2's rules load — every one a read with no side effect the others observe. Issue all three tool calls in a single response, exactly as Step 3 already requires for the agent fan-out, then read their outputs (paging where a file exceeds one read, and those reads can share a response too). The rules load takes `/` — the ref `fetch-pr` just updated; no local-existence probe — **except when the fetch report recorded `baseFetchFailed: true`: drop it from the batch and `git fetch ` first** (on an unresolvable ref `load-rules` reports "no rules found", indistinguishable from a repo that has none, and the review silently enforces nothing). Measured on a real small-PR run: the stretch from `parse-args` to the first agent launch took **7 minutes of wall clock**, one round-trip at a time, on calls that never needed an order. The only orderings that matter: `fetch-pr` before all of them (it creates the worktree and the plan), `repo-context` before `agent-prompt --roster` (the roster and every brief bake the manifest's required agents and context blocks, so building them first silently drops the context), and `agent-prompt --roster` after the rules load (the roster bakes the rules into every brief). - **Fetch PR context** (metadata + already-discussed issues) in one pass: @@ -166,6 +166,8 @@ Based on the parsed `target.type`: - **Do not install dependencies here.** The install belongs to Agent 7, and `qwen review build-test` runs it — nothing before Agent 7 needs `node_modules`: the diff-reading agents read the diff and grep the worktree's _sources_. Run from here it is a **blocking prefix** to the whole fan-out — measured at ~161 seconds on a cold worktree of this repo, because `npm ci` triggers this project's `prepare` hook, which builds and bundles every workspace; run from inside `build-test` (which sets `QWEN_SKIP_PREPARE=1`) the install skips that wasted full build and overlaps the other agents, still reading. At low effort nothing builds or tests at all, so there is no install on that path; medium and high run Agent 7's `build-test`, which does its own install (with `QWEN_SKIP_PREPARE=1`). + - **Attach repository context** at medium or high effort, before `agent-prompt --roster` (and therefore before launching agents): run `qwen review repo-context` with absolute `--plan`, `--worktree`, and `--out` paths. See the repository-context step in the Diff capture section below; for same-repo PRs the manifest is read from the trusted merge base recorded by `fetch-pr`. + - **`file`** (e.g., `src/foo.ts`): - Run `"${QWEN_CODE_CLI:-qwen}" review capture-local --file --target --out .qwen/tmp/qwen-review--plan.json` to get its changes (`--out` is required — see the capture block below for the full form). An **untracked** target file is captured whole (every line reads as added), which is the right frame for a file that does not exist upstream yet. The path is taken relative to **your** working directory and must be inside the repo. - If the plan is empty (the file is tracked and unmodified), read the file and review its current state — see the no-diff branch below @@ -208,6 +210,17 @@ It writes the diff to `.qwen/tmp/qwen-review--diff.txt` and emits the sa - **`untrackedFiles`** — brand-new files, whose contents no `git diff` would have shown. **Name them in the review's summary.** A local review now reads files the user never staged, and the most common untracked-but-unignored file in the wild is a credentials file (`.env`, a key dump). Nothing is filtered — a hardcoded skip-list would reintroduce exactly the silent-skipping this command exists to end — so the user is told instead, and can re-run with `--no-untracked` or fix their `.gitignore`. - **`skippedFiles`** — untracked files that were **not** reviewed, each with a reason: too large, an embedded git repository, a symlink to a directory, a total-budget or file-count cap. **List these under "Not reviewed" in Step 6.** A capture that quietly dropped a file is the bug this command exists to fix; dropping one for a subtler reason would be the same bug wearing a hat. +At **medium or high** effort, for local, file-path, and same-repository PR reviews, attach declarative repository context before `agent-prompt --roster` — the roster and every brief bake this context in, so running it later silently drops the manifest's required agents and guidance (and it is therefore also before launching agents): + +```bash +"${QWEN_CODE_CLI:-qwen}" review repo-context \ + --plan \ + --worktree \ + --out +``` + +Use the captured plan's absolute path and its resolved worktree path. The only manifest is strict JSON at `.qwen/review-context.json`; matching rules add generic domains, related files, tests, configurations, roles, and verification boundaries. For PRs the command reads that manifest from the trusted merge base, never from the PR head — a PR whose base never resolved degrades to a `null` artifact rather than reading the head. Local reviews read it from the current worktree. All three arguments must be absolute so later agent working directories cannot change their meaning. A `null` artifact means no manifest or no matching rule and is not an error; a NON-ZERO exit is fail-closed — stop the review and report it, do not continue with the step silently skipped. Skip this command at low effort and in cross-repository lightweight mode, where there is no trusted local tree. + Do **not** hand-type a `git diff` here. Two reasons, and the second is why this is a command and not a prose recipe: - **The flags.** A user's `color.diff=always` alone makes the diff unparseable, and `diff.mnemonicPrefix` rewrites every path. `capture-local` pins the same ten flags `fetch-pr` pins, from the same constant, so the two capture paths cannot drift into producing diffs that parse differently. @@ -277,7 +290,7 @@ Use **Step 3A** or **Step 3B** as the topology gate in Step 1 decided. The dimen ## Step 3A: Dimension fan-out (small source change) -Launch **14 agents** for same-repo **PR** reviews (Agent 1 has three procedural variants 1a/1b/1c, Agent 3 has three checklist slices 3a/3b/3c, and Agent 6 has three persona variants 6a/6b/6c — each variant counts as a separate parallel agent), plus up to 2 optional diff-specialized finders (Agent 8) when the diff's domain calls for them. For cross-repo lightweight **PR** mode launch **12 agents** — skip Agent 7 (Build & Test) and Agent 1c (Cross-file tracer), since there is no local codebase to build, test, or grep. (Agent 8 finders need only the diff, so the up-to-2 option applies in every mode — lightweight and local included.) Lightweight mode also degrades Agents 1a and 1b, whose briefs assume a source tree: tell them they have the diff ONLY — 1a reviews hunks without enclosing-function reads, and 1b, when it cannot find a deleted invariant re-established because the evidence would live outside the diff, reports the candidate at `Confidence: low` and says the re-establishment could not be checked, instead of asserting it is missing. Step 4's verifiers operate under the same limit, so lightweight-mode findings that depend on unseen source must stay low-confidence (terminal-only) rather than becoming public blockers. **Agent 0 (Issue Fidelity) runs only when the review target is a PR** — a local-diff or file-path review has no PR and no linked issue, so skip Agent 0 and launch **13 agents** (Agents 1a–7). Each agent should focus exclusively on its dimension. (Agent counts are maxima: on a diff with no removed or replaced lines, Agent 1b has nothing to audit and is skipped — one fewer agent.) +Launch **14 agents** for same-repo **PR** reviews (Agent 1 has three procedural variants 1a/1b/1c, Agent 3 has three checklist slices 3a/3b/3c, and Agent 6 has three persona variants 6a/6b/6c — each variant counts as a separate parallel agent), plus up to 2 optional diff-specialized finders (Agent 8) when the diff's domain calls for them. For cross-repo lightweight **PR** mode launch **12 agents** — skip Agent 7 (Build & Test) and Agent 1c (Cross-file tracer), since there is no local codebase to build, test, or grep. (Agent 8 finders need only the diff, so the up-to-2 option applies in every mode — lightweight and local included.) Lightweight mode also degrades Agents 1a and 1b, whose briefs assume a source tree: tell them they have the diff ONLY — 1a reviews hunks without enclosing-function reads, and 1b, when it cannot find a deleted invariant re-established because the evidence would live outside the diff, reports the candidate at `Confidence: low` and says the re-establishment could not be checked, instead of asserting it is missing. Step 4's verifiers operate under the same limit, so lightweight-mode findings that depend on unseen source must stay low-confidence (terminal-only) rather than becoming public blockers. **Agent 0 (Issue Fidelity) runs only when the review target is a PR** — a local-diff or file-path review has no PR and no linked issue, so skip Agent 0 and launch **13 agents** (Agents 1a–7). Each agent should focus exclusively on its dimension. (Agent counts are maxima: on a diff with no removed or replaced lines, Agent 1b has nothing to audit and is skipped — one fewer agent — unless a repository context requires it back, which the `--roster` output below shows.) **At medium effort, launch the reduced set:** skip the three adversarial personas (Agents 6a/6b/6c) and the Agent 8 diff-specialists, launching Agents 0 (PR targets only), 1a, 1b, 1c, 2, 3a, 3b, 3c, 4, 5, and 7 — **11 agents** for a same-repo PR, **10** for a local-diff or file-path review (no Agent 0), **9** for cross-repo lightweight (drop Agent 7 and 1c too, as above). Everything else about 3A is identical — the briefs, the `working_dir` pin, the whiff check, coverage; medium changes only which dimensions launch, not how any agent runs. **Build the roster with `agent-prompt --roster`** — it reads the effort the plan recorded at Step 1 (`plan.effort`), so on a medium plan it omits 6a/6b/6c from the roster it prints (Agent 8 was never in it) and you launch exactly these agents. `check-coverage` (Step 3D) reads the **same** `plan.effort` and requires exactly these too — no flag to pass, and no way for the roster you launched and the gate that checks it to disagree. (The effort lives in the plan, not in a flag, on purpose: a roster a caller could shrink by omitting a flag is a roster that gets shrunk. If Step 1 recorded no effort, the full roster is required, personas included — the fail-safe, not a medium review.) @@ -295,7 +308,7 @@ It prints one labelled block per required agent — which roles this review owes **What it prints is short — a few hundred characters — and it is short on purpose.** It names the agent's role, points at the **brief file** the command just wrote, and lists the `read_file` calls for the diff. The brief itself — the dimension, the finding format, the severity definitions, the project rules — is on disk, and the agent reads it, exactly as it reads the diff. That is not an optimisation. A real run asked to paste twelve prompts cut nineteen hundred characters out of one and then talked its way past the check that caught it (measured; DESIGN.md — The paraphrased roster prompt). What you are asked to carry is now small enough that you will carry it. Copy it; do not retype it. (Agent 8, when you launch one, is the exception — its brief is the one you write, so give it `--whole-diff` and append your domain brief.) -**Which of them you must launch is not your call either — `check-coverage` reads the roster out of the plan** (Step 3D). It knows this diff removes lines, so it expects `1b`; it knows there is a worktree, so it expects `1c` and `7`; it knows there is a pull request, so it expects `0`. A run that skips one is a run with a dimension nobody reviewed, and it will be named. +**Which of them you must launch is not your call either — `check-coverage` reads the roster out of the plan** (Step 3D). It knows this diff removes lines (or a repository context requires the audit back), so it expects `1b`; it knows there is a worktree, so it expects `1c` and `7`; it knows there is a pull request, so it expects `0`. A run that skips one is a run with a dimension nobody reviewed, and it will be named. Why: **the roles this command does not build are the roles that go missing.** Hand-built launches have handed agents prompts naming no diff file at all, and skipped Agent 0 entirely with no check able to see it (measured; DESIGN.md — The roles nobody launched). @@ -335,7 +348,7 @@ Everything below still governs what the agent is asked to do; the command builds **Whole-diff agents — launched alongside the chunk agents, in the same response.** -**Their blocks are already in the `--roster` output above — you have them.** Roles there: `0` (PR reviews), `1b` (when the diff removes anything), `1c`, `test-matrix`, `7` (same-repo), and for a **heavy** file three more, one per checklist slice (their blocks are labelled `Invariant agent A|B|C: … — `). Pass each **verbatim**. To rebuild one for a relaunch: `--role ` (an invariant agent adds `--file `). `check-coverage` derives the same list from the plan and will name any role that did not run. +**Their blocks are already in the `--roster` output above — you have them.** Roles there: `0` (PR reviews), `1b` (when the diff removes anything, or a repository context requires it), `1c`, `test-matrix`, `7` (same-repo), and for a **heavy** file three more, one per checklist slice (their blocks are labelled `Invariant agent A|B|C: … — `). Pass each **verbatim**. To rebuild one for a relaunch: `--role ` (an invariant agent adds `--file `). `check-coverage` derives the same list from the plan and will name any role that did not run. Why: **the chunk agents got the diff and these did not.** In one real 3B run every one of them was launched with no diff path — and these own exactly the classes a chunk agent is structurally blind to (measured; DESIGN.md — The whole-diff agents launched without the diff).