feat(providers): add tokenless diff provider#2467
Conversation
…Agent#2457) - Add publish_file_comments to is_supported() deny-list so describe walkthrough is not silently dropped with inline_file_summary=true - Guard get_diff_files() against path traversal: reject absolute paths and relative paths that resolve outside the repo root; log and fall back to patch-only mode instead of raising - Fix tokenless_diff_mode.md comparison table: local provider inline comments are Not supported (raises NotImplementedError) - Promote page title from H2 to H1 to match other usage-guide pages - Add tests: test_publish_file_comments_not_supported, test_path_traversal_file_not_read
PR Summary by QodoAdd tokenless unified-diff provider (stdin/file) with stdout output Description
Diagram
High-Level Assessment
Files changed (11)
|
Code Review by Qodo
Context used 1. Overlong diff_parsing import line
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
- cli: handle OSError/UnicodeDecodeError when reading --diff-file, failing fast with a clear parser.error instead of an uncaught traceback - diff_parsing: catch the specific UnidiffParseError (not bare Exception) in reconstruct_base_file; keep an empty reconstructed base as "" (never "\n") so downstream extend_patch() treats it as having no original file - diff_provider: narrow exception handling to UnidiffParseError for parsing and (OSError, UnicodeDecodeError) for working-tree reads; resolve diff paths against the repository root (_find_repository_root) instead of the raw CWD so enrichment still works when run from a subdirectory - tests: update add-to-empty base expectation to ""; add regression tests for subdirectory enrichment and missing --diff-file fail-fast; remove unused imports (F401)
|
Code review by qodo was updated up to the latest commit 304384d |
|
Hey @vectorkovacspeter, thanks for taking this! |
|
Thanks for the review. I've addressed all six findings in 304384d:
Added regression tests for the subdirectory case and the missing-file fail-fast; suite is now 25 passing. |
|
Hey @vectorkovacspeter, Could you also please address the first 3 that Qodo hasn't marked as resolved? |
… mode Addresses the new findings raised after the previous fix round: - improve crash (correctness): DiffGitProvider now implements publish_code_suggestions()/publish_code_suggestion() to render suggestions as a markdown document to stdout/--output instead of raising NotImplementedError, so `--stdin/--diff-file improve` works as documented - CWD file-disclosure risk (security): when no .git repository root is detected, working-tree enrichment is disabled (patch-only) instead of reading files from an arbitrary current directory - broad except (reliability): _write_output() now catches only (OSError, UnicodeError) and re-raises, since --output is an explicit request whose failure must not be swallowed - style: double quotes for the new CLI args; wrapped the long publish_inline_comment signature to satisfy Ruff - docs: note that improve renders suggestions as markdown in this mode - tests: add coverage for code-suggestion rendering (and empty no-op), no-repo-root patch-only mode; harden the path-traversal sentinel with a real .git root so it isolates the traversal guard
|
Done — pushed dfb19b2. Addressed the new findings:
Added tests for the suggestion rendering and the no-repo-root path; suite is 28 passing. |
|
Code review by qodo was updated up to the latest commit dfb19b2 |
|
Oh man, this bot is a hard one , but i working on it 😄 |
…ff mode - incremental crash (correctness): DiffGitProvider.get_incremental_commits() now disables incremental mode (-i has no meaning for a standalone diff), preventing a TypeError when PRReviewer takes len() of an empty commits_range - extra-config override (correctness): provider selection now forces the diff provider whenever diff content is loaded, so an extra/repo config setting config.git_provider can no longer silently derail tokenless mode - suppressed output (correctness): CLI diff mode forces config.publish_output =True so stdout/--output is always produced even if publishing was disabled - global mutation (reliability): removed the dead pr_reviewer.inline_code_comments=False write in __init__ (never read anywhere; was copied from LocalGitProvider) - test isolation (reliability): added an autouse fixture to each diff test module that snapshots and restores the mutated global settings keys - tests: added coverage for incremental-disabled, diff-content-forces-provider, and publish_output forcing
|
Pushed a05bce3 addressing the new findings:
|
|
Code review by qodo was updated up to the latest commit a05bce3 |
…und 4) - patch misparsed as hunks (correctness): the stored FilePatchInfo.patch was str(pf) from unidiff, which keeps the 'diff --git'/'index'/'---'/'+++' headers before the first '@@'. The shared hunk/line-number converter treats '+'/'-' lines as content, so those headers were emitted as a bogus leading hunk with invalid line numbers into the LLM prompt. Added to_hunk_only_patch() and normalize f.patch after base reconstruction (which still needs the full headers to parse), matching platform providers. - isort (I001): collapsed the two-name diff_parsing import to one line and sorted imports in the diff test modules; verified with ruff. - test isolation: tests now mutate settings only through an autouse `cfg` fixture that snapshots and restores the diff-mode keys (covers run()'s indirect mutations too); no bare get_settings().set() in test bodies. - test style: replaced `assert False` with pytest.raises (B011). - tests: added a hunk-only-patch assertion.
|
Pushed ae473c4. Addressed the findings:
32 tests pass. Note: I scoped changes to this feature and left pre-existing repo lint (e.g. the duplicate GiteaProvider import, some long lines) untouched to keep the diff focused — happy to fold those in separately if you'd like. |
|
Code review by qodo was updated up to the latest commit ae473c4 |
|
@naorpeled all of Qodo's findings are now resolved — on the latest push it finally came back empty-handed. 🎉 Genuinely impressed with that review bot, by the way: it caught a couple of real ones I'd have shipped (the improve crash and the diff-metadata-misparsed-as-hunks bug were proper catches). It's also… thorough. Four rounds deep I'm fairly sure it would flag the Mona Lisa for an unsorted import. 😄 No complaints though — the code is better for it. Ready for your review whenever you have a chance, and happy to make any further changes you'd like. |
naorpeled
changed the title
|
Hey @vectorkovacspeter, |
Yeah not the best name. stdin may not cover input diff file based method. Maybe just call it diff provider or offline provider or something like this... |
What about plain-diff? |
Approved 😁 thanks for asking. |
|
Hey @vectorkovacspeter, Thanks in advance! |
…he-PR-Agent#2457) Rename the provider per maintainer request (@naorpeled): registry key "diff" -> "plain-diff", DiffGitProvider -> PlainDiffGitProvider, settings namespace diff.* -> plain_diff.*, docs/tests/CLI help updated. Also drop a pre-existing duplicate GiteaProvider import and fix import ordering.
|
Done, @naorpeled — renamed to
Kept the |
|
Code review by qodo was updated up to the latest commit c362734 |
…he-PR-Agent#2457) apply_repo_settings() runs after cli.run() forces config.publish_output=True and can overwrite it back to False from an extra config (--extra_config_url), because it merges section keys with overwrite. Since plain-diff mode's only output channel is stdout/--output and all tools gate publishing on this flag, the review could silently produce no output. PlainDiffGitProvider is constructed after apply_repo_settings, so re-assert config.publish_output=True in its __init__. Adds a regression test. (Note: the repo-local .pr_agent.toml path cannot reach this in plain-diff mode since get_repo_settings() returns None; the extra-config path can.)
|
Addressed the new Qodo finding (#1 "Diff output can be suppressed") in Nice catch, but worth splitting the two override vectors in
Fix: |
| assert files[0].head_file != "", "head_file should be populated from the working-tree file" | ||
| assert "line2-changed" in files[0].head_file | ||
|
|
||
| # base_file must be reconstructed (non-empty) | ||
| assert files[0].base_file != "", "base_file should be reconstructed by reversing the patch" |
There was a problem hiding this comment.
1. Overlong asserts in e2e test 📘 Rule violation ⚙ Maintainability
Several new assertions use single lines that exceed the project's 120-character limit, which can cause Ruff/CI formatting failures. This reduces consistency and increases review churn for future edits.
Agent Prompt
## Issue description
New test lines exceed the 120-character line-length limit required by the project's Ruff formatting standards.
## Issue Context
This can fail formatting/lint checks (e.g., E501) and makes diffs harder to review.
## Fix Focus Areas
- tests/unittest/test_plain_diff_mode_e2e.py[80-84]
- tests/unittest/test_plain_diff_mode_e2e.py[162-162]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| # A hunk with no file header triggers UnidiffParseError inside parse_unified_diff, | ||
| # which the provider must re-raise as ValueError with a clear message. | ||
| cfg("plain_diff.content", "@@ -1,3 +1,3 @@\n line1\n-line2\n+line2-changed\n line3\n") | ||
| cfg("plain_diff.output_path", None) |
There was a problem hiding this comment.
2. Overlong diff string literal 📘 Rule violation ⚙ Maintainability
A new test embeds a long diff string in a single line that exceeds the 120-character limit, which can trigger Ruff/CI failures. It should be wrapped or split into concatenated strings for compliance with project formatting standards.
Agent Prompt
## Issue description
A long inline diff string exceeds the project's 120-character line-length requirement.
## Issue Context
This can fail Ruff/CI checks and reduces readability.
## Fix Focus Areas
- tests/unittest/test_plain_diff_provider.py[165-171]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| def get_issue_comments(self): | ||
| raise NotImplementedError("Issue comments are not supported by the plain-diff provider") | ||
|
|
There was a problem hiding this comment.
3. Improve crashes on comments 🐞 Bug ☼ Reliability
PlainDiffGitProvider.get_issue_comments() raises NotImplementedError, but the /improve flow calls git_provider.get_issue_comments() unconditionally when building persistent suggestion history, so --stdin/--diff-file improve can crash before emitting output.
Agent Prompt
### Issue description
`PlainDiffGitProvider.get_issue_comments()` raises `NotImplementedError`, but `PRCodeSuggestions.publish_persistent_comment_with_history()` calls `list(git_provider.get_issue_comments())` without a capability guard. In plain-diff mode this can abort the supported `improve` command.
### Issue Context
Plain-diff mode relies on stdout/`--output` as its only sink; a crash here means the user gets no suggestions at all.
### Fix Focus Areas
- pr_agent/git_providers/plain_diff_provider.py[202-204]
- pr_agent/tools/pr_code_suggestions.py[272-276]
### Expected fix
- Make `PlainDiffGitProvider.get_issue_comments()` return an empty iterable (e.g., `return []`) instead of raising, so persistent-history code can safely treat “no comments” as the default.
- (Optional hardening) In `publish_persistent_comment_with_history`, catch `NotImplementedError`/`Exception` around `get_issue_comments()` and treat it as `[]` to avoid similar failures for future providers.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
|
Code review by qodo was updated up to the latest commit d0f4197 |
…omments (The-PR-Agent#2457) Two Qodo findings on the renamed provider: - ask routing (correctness): PRQuestions builds its provider via get_git_provider(), which lacked the plain-diff override that get_git_provider_with_context() already has. If an extra config overwrote config.git_provider after apply_repo_settings(), 'ask' could route to a hosted provider. Add the same 'force plain-diff when plain_diff.content is set' keying to get_git_provider(). - issue comments (reliability): get_issue_comments() raised NotImplementedError, which the improve persistent-comment path caught and logged as a spurious traceback on every run. Return [] instead (a raw diff has no comment history); suggestions still reach stdout. Adds regression tests. 35 diff-mode tests pass.
|
Went through the four new findings — two fixed in ✅ #3 Improve crashes on comments (fixed). Slight correction: it doesn't actually crash — ✅ #4 Ask bypasses diff provider (fixed). Good catch. ❌ #1 Overlong asserts / ❌ #2 Overlong diff literal — false positives. These lines are within the 120-char limit (
35 diff-mode tests green ✅ |
|
Code review by qodo was updated up to the latest commit b736fbd |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| never suppressed by a config/env that disabled publishing.""" | ||
| import io | ||
|
|
||
| import pr_agent.cli as cli |
|
Hey @vectorkovacspeter, Can you please also address "Improve crashes on comments"? |
CodeQL flagged test_cli_plain_diff_mode.py importing pr_agent.cli both as
`from pr_agent.cli import run, set_parser` and `import pr_agent.cli as cli`.
The module import only existed to monkeypatch PRAgent; switch to a string
target (`monkeypatch.setattr("pr_agent.cli.PRAgent", ...)`) and drop the
duplicate import. 35 diff-mode tests pass.
|
Hey @naorpeled — good news, that one's already fixed! It went in with Quick recap of what I meant earlier: the finding's title ("crashes") isn't literally accurate — def get_issue_comments(self):
# A raw diff has no issue-comment history. Return an empty iterable rather
# than raising: the improve persistent-comment path calls this without a
# capability guard, and treating "no comments" as the default lets it fall
# through to publishing the suggestions to stdout without a spurious
# traceback in the log.
return []No more spurious traceback, output unchanged, plus a regression test ( Also just pushed 35 diff-mode tests green ✅ Ready for the release whenever you are 🚀 |
| from pr_agent.config_loader import _find_repository_root, get_settings | ||
| from pr_agent.git_providers.diff_parsing import parse_unified_diff, reconstruct_base_file, to_hunk_only_patch | ||
| from pr_agent.git_providers.git_provider import GitProvider |
There was a problem hiding this comment.
1. Overlong diff_parsing import line 📘 Rule violation ⚙ Maintainability
The new import line in plain_diff_provider.py exceeds the repo’s 120-character limit and is likely to fail Ruff (E501) / isort formatting expectations. This can break CI/lint checks and reduce maintainability.
Agent Prompt
## Issue description
`pr_agent/git_providers/plain_diff_provider.py` includes a long single-line import that exceeds the configured Ruff line-length (120) and may fail linting.
## Issue Context
Ruff is configured with `line-length = 120` and `lint.select` includes `E` and `I001` (isort checks). New code should comply to avoid CI failures.
## Fix Focus Areas
- pr_agent/git_providers/plain_diff_provider.py[8-10]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
|
Code review by qodo was updated up to the latest commit 6ce6de5 |
|
Checked this one — it's a false positive (the
Worth noting the suggested fix would actually break the build: isort/Ruff only wraps an import when it exceeds (Same category as the two earlier "overlong line" flags on the e2e/provider tests — those were 95/108/90 chars, also within 120.) Happy to wrap anything if you'd prefer a tighter house limit, @naorpeled, but under the current config these are compliant. |
3 participants
feature: tokenless local diff mode (
--stdin/--diff-file)Closes #2457
Summary
Adds a purely local, stateless way to run PR-Agent against a unified diff supplied on
stdinor from a file — no git-platform API token and no PR URL required. Thegenerated review/improvements/description are printed to
stdout(markdown), and canadditionally be written to a file with
--output.Supported commands:
review,improve,describe,ask.Why
Today PR-Agent requires a platform-specific token (GitHub PAT/App, GitLab token, …)
because it talks to the hosting platform's API to fetch PR data and publish comments.
In security-first / restricted environments, teams often manage repositories over SSH
and deliberately avoid issuing HTTP access tokens, since every static token is an extra
credential that can leak.
This feature lets those users get AI code reviews while keeping a strict, token-free
posture:
host's API is blocked.
pre-commit/pre-pushhooks.How to use
--stdin--diff-file <path>--output <path>The provider is selected automatically when
--stdinor--diff-fileis present, andthe usual
--pr_url/--issue_urlrequirement is bypassed.Any LLM supported by PR-Agent works, including a fully local / OpenAI-compatible
endpoint (e.g. Ollama or vLLM) for an end-to-end air-gapped setup.
How it works
git_provider = "diff"(DiffGitProvider), parses the unifieddiff with the
unidifflibrary into PR-Agent'sFilePatchInfoobjects.reverse-applies the diff to reconstruct the original (base) file, so each hunk
gets full surrounding context (
extend_patch) — matching the quality of the normalAPI path rather than only the diff's few context lines.
back to a patch-only review instead of producing a wrong base file.
stdout, and to--output <path>when provided.Design notes / scope
branch-based
localprovider (which requires a clean repo and a target branch). Thelocalprovider is left untouched.committable code suggestions, labels, reactions) are no-ops /
NotImplementedError,mirroring
LocalGitProvider;is_supported()reports them as unsupported so thetools degrade gracefully.
..-escapes from the diff are rejected — so an untrusted diff cannot make the toolread files outside the repo.
unidiff(MIT), added torequirements.txt.Testing
New tests under
tests/unittest/:test_diff_parsing.py— parsing (modify/add/delete/rename/binary) and base-filereverse-apply (success, drift → patch-only, multi-hunk, CRLF, EOF-newline).
test_diff_provider.py— provider behavior, stdout/file output, temporary-commentsuppression, malformed-diff guard, capability gating, and a path-traversal guard
(with a sentinel test verified to fail if the guard is removed).
test_cli_diff_mode.py— CLI flag parsing.test_diff_mode_e2e.py— base reconstruction from the working tree and a mocked-LLMrun through
PRReviewer.All diff-feature tests pass. Unrelated pre-existing test-collection errors in some
test_mosaico_*files (optional dependencies not installed) are not affected by thischange.
Documentation
Adds
docs/docs/usage-guide/tokenless_diff_mode.mdand a navigation entry indocs/mkdocs.yml, including usage, the working-tree vs. patch-only quality trade-off,and how this differs from the
localprovider.AI assistance disclosure
In the interest of transparency: this contribution was developed with substantial help
from an AI coding assistant — it contains AI-generated and/or AI-assisted code,
tests, and documentation. All of it was reviewed, tested, and validated by me before
submission, and I take full responsibility for the contents of this PR. I'm happy to
adjust anything to fit the project's conventions and review feedback.