feat(agent-runner): add diff review mode to review method #218

Merged

stack72 merged 2 commits from feat/agent-runner-diff-review-mode into main 2026-08-23 10:51:17 +00:00

Conversation 4 Commits 2 Files changed 7 +248 -62

stack72 commented 2026-08-23 10:21:14 +00:00

Owner

Copy link

Summary

• Add optional diff input to the review method schema that passes PR diff content directly in the prompt, replacing the file list. The agent reviews inline changes without needing to Read each file, cutting review time significantly for large changesets.
• When diff is provided, agent tools are restricted to Read, Grep, Glob, and Bash (Bash needed for the tee output mechanism). The files input becomes optional — callers must provide one or the other (validated at runtime).
• Bump model version to 2026.08.23.1 with an upgrade entry for the new optional field.

Files changed

File	Change
schemas.ts	files now optional, added diff: z.string().optional()
review.ts	buildReviewPrompt takes discriminated mode param, new injectDiffModeTools, input validation
agent_runner.ts	Version bump, upgrade entry
manifest.yaml	Version bump, updated description
review_test.ts	New — 10 tests for buildReviewPrompt, injectDiffModeTools, schema shapes
runner_test.ts	Replaced stale buildTestPrompt mirror with real import, added diff-mode test
agent_runner_test.ts	Fixed files?.length for optional type, removed stale comment

Known limitation

Codex provider's config schema doesn't define allowedTools, so the diff-mode tool restriction silently has no effect for Codex. The diff content still lands in the prompt (speed benefit preserved), just no tool scoping. Not a regression — Codex had no tool restrictions in file-list mode either.

Test plan

• All 86 tests pass (deno test --allow-read --allow-write --allow-net --allow-env --allow-run)
• Type-check clean (deno check)
• Lint and format clean (deno lint, deno fmt --check)
• File-list mode (existing behavior) unchanged — callers passing files work identically
• Diff mode constructs correct prompt with inline diff content
• Validation rejects calls with neither files nor diff
• Tool injection respects explicit allowedTools override
• runner_test.ts uses real buildReviewPrompt — no stale mirrors

🤖 Generated with Claude Code

https://claude.ai/code/session_01M1Sjoft1zdsi2chnbRDKrL

## Summary - Add optional `diff` input to the `review` method schema that passes PR diff content directly in the prompt, replacing the file list. The agent reviews inline changes without needing to Read each file, cutting review time significantly for large changesets. - When `diff` is provided, agent tools are restricted to `Read`, `Grep`, `Glob`, and `Bash` (Bash needed for the `tee` output mechanism). The `files` input becomes optional — callers must provide one or the other (validated at runtime). - Bump model version to `2026.08.23.1` with an upgrade entry for the new optional field. ## Files changed | File | Change | |------|--------| | `schemas.ts` | `files` now optional, added `diff: z.string().optional()` | | `review.ts` | `buildReviewPrompt` takes discriminated `mode` param, new `injectDiffModeTools`, input validation | | `agent_runner.ts` | Version bump, upgrade entry | | `manifest.yaml` | Version bump, updated description | | `review_test.ts` | New — 10 tests for `buildReviewPrompt`, `injectDiffModeTools`, schema shapes | | `runner_test.ts` | Replaced stale `buildTestPrompt` mirror with real import, added diff-mode test | | `agent_runner_test.ts` | Fixed `files?.length` for optional type, removed stale comment | ## Known limitation Codex provider's config schema doesn't define `allowedTools`, so the diff-mode tool restriction silently has no effect for Codex. The diff content still lands in the prompt (speed benefit preserved), just no tool scoping. Not a regression — Codex had no tool restrictions in file-list mode either. ## Test plan - [x] All 86 tests pass (`deno test --allow-read --allow-write --allow-net --allow-env --allow-run`) - [x] Type-check clean (`deno check`) - [x] Lint and format clean (`deno lint`, `deno fmt --check`) - [x] File-list mode (existing behavior) unchanged — callers passing `files` work identically - [x] Diff mode constructs correct prompt with inline diff content - [x] Validation rejects calls with neither `files` nor `diff` - [x] Tool injection respects explicit `allowedTools` override - [x] `runner_test.ts` uses real `buildReviewPrompt` — no stale mirrors 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_01M1Sjoft1zdsi2chnbRDKrL

stack72 added 1 commit 2026-08-23 10:21:14 +00:00

feat(agent-runner): add diff review mode to review method ... ff5c9da32d

Add an optional `diff` input to the review method that passes PR diff
content directly in the prompt instead of a file list. This avoids the
agent needing to Read each file individually, significantly reducing
review time for large changesets.

When `diff` is provided:
- The diff content replaces the file list in the prompt
- Agent tools are restricted to Read, Grep, Glob, and Bash
- The `files` input becomes unnecessary

The `files` input is now optional — callers must provide either `files`
or `diff` (validated at runtime in executeReview).

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01M1Sjoft1zdsi2chnbRDKrL

stack72 commented 2026-08-23 10:29:19 +00:00

Author

Owner

Copy link

Code Review

Blocking Issues

None.

Suggestions

1. diff: z.string() should have a min(1) constraint (schemas.ts).
   An empty-string diff: "" passes schema validation today, but !args.diff is
   truthy for "", so the runtime guard in executeReview throws
   "Either 'files' or 'diff' must be provided" even though the caller did supply
   the field. If both diff: "" and files: [...] are passed, the empty diff is
   silently ignored and files mode runs instead — surprising to callers who expect
   diff mode.

   // schemas.ts
   diff: z.string().min(1).optional()
   

2. Scope Bash in DIFF_MODE_ALLOWED_TOOLS to the write operation
   (review.ts:196).
   Bash is included so the agent can write its result via tee. Claude Code
   supports tool patterns like "Bash(tee:*)" that restrict which shell commands
   are permitted. Using the pattern instead of bare "Bash" minimises blast radius
   if a malicious diff contains prompt-injection instructions that try to abuse the
   shell.

   const DIFF_MODE_ALLOWED_TOOLS = ["Read", "Grep", "Glob", "Bash(tee:*)"];
   

   This is a nice-to-have hardening step, not a requirement — the existing
   behaviour is intentional and documented.

## Code Review ### Blocking Issues None. ### Suggestions 1. **`diff: z.string()` should have a `min(1)` constraint** (`schemas.ts`). An empty-string `diff: ""` passes schema validation today, but `!args.diff` is truthy for `""`, so the runtime guard in `executeReview` throws `"Either 'files' or 'diff' must be provided"` even though the caller did supply the field. If both `diff: ""` and `files: [...]` are passed, the empty diff is silently ignored and files mode runs instead — surprising to callers who expect diff mode. ```ts // schemas.ts diff: z.string().min(1).optional() ``` 2. **Scope `Bash` in `DIFF_MODE_ALLOWED_TOOLS` to the write operation** (`review.ts:196`). `Bash` is included so the agent can write its result via `tee`. Claude Code supports tool patterns like `"Bash(tee:*)"` that restrict which shell commands are permitted. Using the pattern instead of bare `"Bash"` minimises blast radius if a malicious diff contains prompt-injection instructions that try to abuse the shell. ```ts const DIFF_MODE_ALLOWED_TOOLS = ["Read", "Grep", "Glob", "Bash(tee:*)"]; ``` This is a nice-to-have hardening step, not a requirement — the existing behaviour is intentional and documented.

stack72 commented 2026-08-23 10:32:06 +00:00

Author

Owner

Copy link

Adversarial Review

Medium

1. review.ts:198-219 -- injectDiffModeTools can create allowedTools/disallowedTools conflict with no validation.

   When the merged provider config already has disallowedTools but no allowedTools, the function injects allowedTools alongside the existing disallowedTools. If a profile intentionally blocks Bash via disallowedTools, the result is both allowedTools and disallowedTools containing Bash.

   Whether Bash is actually available depends entirely on how the provider resolves the conflict. This code does not check for or prevent the contradiction. In Claude Code, disallowedTools takes precedence, so this is likely safe in practice, but the behavior is implicit and undocumented.

   Breaking scenario: A future provider (or a change to Claude Code semantics) that treats allowedTools as authoritative over disallowedTools would silently grant Bash access to a review agent that was explicitly denied it.

   Suggested fix: Before injecting, filter out any tools present in disallowedTools using a Set to avoid the contradiction.

Low

1. review.ts:35-37,62 -- When both files and diff are provided, diff silently wins.

   The validation on line 35 only checks that at least one is present. When both are supplied, line 62 silently uses diff and discards files. This is undocumented and untested. A caller providing both inputs will not know that files was ignored. Not harmful, but could confuse CI pipeline authors debugging unexpected review scope.

2. agent_runner_test.ts:502 -- Misleading test name.

   The test is named "review args require either profile or promptFile" but actually demonstrates the opposite: it parses input with neither profile nor promptFile and asserts success. The old explanatory comment was removed in this PR. The test validates the schema correctly (the constraint is enforced at runtime, not schema level), but the name will mislead anyone reading the test suite.

3. review.ts:35 -- Whitespace-only diff passes validation.

   args.diff is checked with truthiness, so a whitespace-only diff string passes validation and enters diff mode with no meaningful content. The agent would receive a prompt with an effectively empty diff. Unlikely in practice but trivially prevented with a trim check.

Verdict

PASS -- The changes are clean and well-tested. The diff mode feature is a straightforward extension of the existing review flow. The discriminated union approach for mode is sound, the runtime validation correctly compensates for the schema inability to express the files-XOR-diff constraint, and injectDiffModeTools correctly respects explicit allowedTools overrides. The medium finding is a defense-in-depth concern, not a blocking issue in current provider implementations. Test coverage for the new helpers (buildReviewPrompt, injectDiffModeTools) is thorough, including edge cases like empty configs and cross-provider key preservation.

## Adversarial Review ### Medium 1. **review.ts:198-219 -- injectDiffModeTools can create allowedTools/disallowedTools conflict with no validation.** When the merged provider config already has disallowedTools but no allowedTools, the function injects allowedTools alongside the existing disallowedTools. If a profile intentionally blocks Bash via disallowedTools, the result is both allowedTools and disallowedTools containing Bash. Whether Bash is actually available depends entirely on how the provider resolves the conflict. This code does not check for or prevent the contradiction. In Claude Code, disallowedTools takes precedence, so this is likely safe in practice, but the behavior is implicit and undocumented. **Breaking scenario:** A future provider (or a change to Claude Code semantics) that treats allowedTools as authoritative over disallowedTools would silently grant Bash access to a review agent that was explicitly denied it. **Suggested fix:** Before injecting, filter out any tools present in disallowedTools using a Set to avoid the contradiction. ### Low 1. **review.ts:35-37,62 -- When both files and diff are provided, diff silently wins.** The validation on line 35 only checks that at least one is present. When both are supplied, line 62 silently uses diff and discards files. This is undocumented and untested. A caller providing both inputs will not know that files was ignored. Not harmful, but could confuse CI pipeline authors debugging unexpected review scope. 2. **agent_runner_test.ts:502 -- Misleading test name.** The test is named "review args require either profile or promptFile" but actually demonstrates the opposite: it parses input with neither profile nor promptFile and asserts success. The old explanatory comment was removed in this PR. The test validates the schema correctly (the constraint is enforced at runtime, not schema level), but the name will mislead anyone reading the test suite. 3. **review.ts:35 -- Whitespace-only diff passes validation.** args.diff is checked with truthiness, so a whitespace-only diff string passes validation and enters diff mode with no meaningful content. The agent would receive a prompt with an effectively empty diff. Unlikely in practice but trivially prevented with a trim check. ### Verdict **PASS** -- The changes are clean and well-tested. The diff mode feature is a straightforward extension of the existing review flow. The discriminated union approach for mode is sound, the runtime validation correctly compensates for the schema inability to express the files-XOR-diff constraint, and injectDiffModeTools correctly respects explicit allowedTools overrides. The medium finding is a defense-in-depth concern, not a blocking issue in current provider implementations. Test coverage for the new helpers (buildReviewPrompt, injectDiffModeTools) is thorough, including edge cases like empty configs and cross-provider key preservation.

stack72 added 1 commit 2026-08-23 10:42:09 +00:00

fix(agent-runner): address PR review feedback ... c8b82e430e

- Add min(1) to diff schema to reject empty strings
- Scope Bash tool to Bash(tee:*) instead of unrestricted Bash
- Filter injected allowedTools against disallowedTools to prevent
  conflicts (e.g. profile blocks Bash via disallowedTools)
- Fix misleading test name "review args require either profile or
  promptFile" → "review args schema accepts minimal input"
- Add tests for empty diff rejection and disallowedTools filtering

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01M1Sjoft1zdsi2chnbRDKrL

stack72 commented 2026-08-23 10:49:33 +00:00

Author

Owner

Copy link

Code Review

The PR adds a diff input mode to the review method, allowing callers to pass an inline diff string instead of a file list. The agent reviews the diff directly without reading files, and tool access is narrowed to Read, Grep, Glob, and Bash(tee:*) in that mode. The implementation is clean and well-tested.

Blocking Issues

None.

Suggestions

1. injectDiffModeTools injects Claude-specific tool names for all providers. When the provider is codex, the function still writes allowedTools into the codex config key. Codex uses sandbox/approvalPolicy rather than allowedTools, so this is silently ignored and diff mode with Codex has no tool restriction applied. If Codex diff-mode restriction matters, a guard returning early for non-Claude providers (with a comment) would make the intent explicit.

2. Undocumented behavior when both files and diff are provided. The code resolves the conflict via args.diff ? ... : ... so diff silently wins. The schema field descriptions say each is "Required when the other is not provided" without mentioning precedence. A z.refine() enforcing mutual exclusivity, or at minimum a comment in the schema, would prevent caller confusion.

3. No test for the runtime guard in executeReview. The guard that rejects calls where neither files nor diff is set is the only place that enforces that invariant — the schema itself permits neither field. The existing harness in agent_runner_test.ts wires up a mock ctx without needing a real agent binary, so a test calling executeReview with neither field and asserting the thrown error message would close this coverage gap at low cost.

## Code Review The PR adds a `diff` input mode to the `review` method, allowing callers to pass an inline diff string instead of a file list. The agent reviews the diff directly without reading files, and tool access is narrowed to `Read`, `Grep`, `Glob`, and `Bash(tee:*)` in that mode. The implementation is clean and well-tested. ### Blocking Issues None. ### Suggestions 1. **`injectDiffModeTools` injects Claude-specific tool names for all providers.** When the provider is `codex`, the function still writes `allowedTools` into the `codex` config key. Codex uses `sandbox`/`approvalPolicy` rather than `allowedTools`, so this is silently ignored and diff mode with Codex has no tool restriction applied. If Codex diff-mode restriction matters, a guard returning early for non-Claude providers (with a comment) would make the intent explicit. 2. **Undocumented behavior when both `files` and `diff` are provided.** The code resolves the conflict via `args.diff ? ... : ...` so `diff` silently wins. The schema field descriptions say each is "Required when the other is not provided" without mentioning precedence. A `z.refine()` enforcing mutual exclusivity, or at minimum a comment in the schema, would prevent caller confusion. 3. **No test for the runtime guard in `executeReview`.** The guard that rejects calls where neither `files` nor `diff` is set is the only place that enforces that invariant — the schema itself permits neither field. The existing harness in `agent_runner_test.ts` wires up a mock `ctx` without needing a real agent binary, so a test calling `executeReview` with neither field and asserting the thrown error message would close this coverage gap at low cost.

stack72 commented 2026-08-23 10:50:32 +00:00

Author

Owner

Copy link

Adversarial Review

Medium

1. Silent diff precedence when both diff and files are provided - review.ts:62,71

   The schema (ReviewArgsSchema) allows both diff and files to be provided simultaneously. The runtime validation on line 35 only checks that at least one is present. When both are supplied, diff silently wins. The diff-mode tool injection also kicks in, restricting tool access.

   Breaking example: A caller passes both diff and files expecting the agent to review both the diff context and explore the listed files. Instead, files is silently ignored, tool access is restricted to diff-mode tools, and the agent never reads the listed files.

   Suggested fix: Either (a) throw an error if both diff and files are provided, (b) log a warning that files is being ignored in diff mode, or (c) document the precedence in the diff field schema description.

Low

1. injectDiffModeTools injects Claude-specific allowedTools for any provider - review.ts:196-226

   DIFF_MODE_ALLOWED_TOOLS includes Bash(tee:*) which is Claude Code CLI scoping syntax. When providerName is codex, the function injects codex-keyed allowedTools. The codex provider likely ignores unknown config keys, so this is harmless in practice, but it is conceptually leaky.

   Suggested fix: Guard the injection behind a provider check, e.g. early-return when providerName is not claude.

2. mergeProviderConfig is a shallow merge - review.ts:333-341

   mergeProviderConfig uses spread-based shallow merging. If both a profile and args specify providerConfig with the same provider key, the override entirely replaces the profile provider block rather than deep-merging them. This is likely intentional but callers may expect deep merging of provider sub-configs.

Verdict

PASS - The PR is well-structured and backwards-compatible. The files field is correctly made optional, the new diff mode is cleanly integrated with appropriate tool scoping, and the test coverage is thorough. The medium finding (silent precedence of diff over files) is a minor API clarity issue, not a correctness bug. No critical or high severity findings.

## Adversarial Review ### Medium 1. **Silent diff precedence when both diff and files are provided** - review.ts:62,71 The schema (ReviewArgsSchema) allows both diff and files to be provided simultaneously. The runtime validation on line 35 only checks that at least one is present. When both are supplied, diff silently wins. The diff-mode tool injection also kicks in, restricting tool access. **Breaking example:** A caller passes both diff and files expecting the agent to review both the diff context and explore the listed files. Instead, files is silently ignored, tool access is restricted to diff-mode tools, and the agent never reads the listed files. **Suggested fix:** Either (a) throw an error if both diff and files are provided, (b) log a warning that files is being ignored in diff mode, or (c) document the precedence in the diff field schema description. ### Low 1. **injectDiffModeTools injects Claude-specific allowedTools for any provider** - review.ts:196-226 DIFF_MODE_ALLOWED_TOOLS includes Bash(tee:*) which is Claude Code CLI scoping syntax. When providerName is codex, the function injects codex-keyed allowedTools. The codex provider likely ignores unknown config keys, so this is harmless in practice, but it is conceptually leaky. **Suggested fix:** Guard the injection behind a provider check, e.g. early-return when providerName is not claude. 2. **mergeProviderConfig is a shallow merge** - review.ts:333-341 mergeProviderConfig uses spread-based shallow merging. If both a profile and args specify providerConfig with the same provider key, the override entirely replaces the profile provider block rather than deep-merging them. This is likely intentional but callers may expect deep merging of provider sub-configs. ### Verdict **PASS** - The PR is well-structured and backwards-compatible. The files field is correctly made optional, the new diff mode is cleanly integrated with appropriate tool scoping, and the test coverage is thorough. The medium finding (silent precedence of diff over files) is a minor API clarity issue, not a correctness bug. No critical or high severity findings.

stack72 merged commit d6677d0b35 into main 2026-08-23 10:51:17 +00:00

stack72 deleted branch feat/agent-runner-diff-review-mode 2026-08-23 10:51:17 +00:00

stack72 referenced this pull request from a commit 2026-08-23 10:51:18 +00:00

feat(agent-runner): add diff review mode to review method (#218)

Sign in to join this conversation.

Reviewers

No reviewers

Labels

Clear labels

No items

No labels

Milestone

Clear milestone

No items

No milestone

Assignees

Clear assignees

No assignees

1 participant

Notifications

Subscribe

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference

swamp-club/swamp-extensions!218

No description provided.