fix(server): prevent stale diff in practice review pipeline - #981
Conversation
When a student pushes multiple commits to an MR branch, the local git
clone may have a stale branch ref from an earlier push. The review job
fires but computes a diff against the wrong commit because:
1. ensureRepositoryAvailable only checks existence, never fetches
2. resolveDiffRange Strategy 1 uses origin/{sourceBranch} without
verifying it matches the expected headSha
3. Result: agent receives a stale diff instead of the full MR diff
Fix by:
- Fetching latest refs from origin before computing the diff
- Validating that Strategy 1's branch ref matches the expected headSha
before trusting it, falling through to SHA-based strategies if stale
- Adding diff quality logging (strategy used, range, +/- line counts)
so stale-diff incidents are detectable in production logs
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughRefreshes remote refs before computing PR diffs, validates branch-resolved commits against expected head SHA (falls back to SHA-based resolution if stale), enriches pre-computed diff logging with strategy, resolved range, and added/removed line counts, and makes GitLab token usage optional. Changes
Sequence Diagram(s)sequenceDiagram
participant AgentJob
participant PullRequestReviewHandler
participant GitLabTokenService
participant GitRemote
participant DiffStorage
AgentJob->>PullRequestReviewHandler: computeAndStoreDiff(job)
PullRequestReviewHandler->>GitLabTokenService: getToken(workspaceId) (optional)
alt token available
PullRequestReviewHandler->>GitRemote: git fetch origin (with token)
else token missing or fetch fails
PullRequestReviewHandler->>GitRemote: git fetch origin (unauthenticated or skipped)
end
PullRequestReviewHandler->>PullRequestReviewHandler: resolveDiffRange(headSha, sourceBranch)
PullRequestReviewHandler->>PullRequestReviewHandler: compute unified diff (determine strategy, count added/removed lines)
PullRequestReviewHandler->>DiffStorage: store pre-computed diff + metadata (strategy, resolved range, line counts)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
Fixes an issue in the PR practice review pipeline where diffs could be computed against stale local refs when multiple commits are pushed in quick succession, leading to incorrect review context.
Changes:
- Added a pre-diff
git fetch originto refresh local refs before resolving and computing the diff. - Hardened
resolveDiffRangeStrategy 1 by validatingorigin/{sourceBranch}against the expectedheadSha, falling back to SHA-based strategies when stale. - Added production logging for diff strategy, resolved range, and basic +/− line metrics to help detect stale-diff incidents.
| // Fetch latest refs from remote before computing the diff. | ||
| // Without this, the local clone may have stale branch refs from an earlier push, | ||
| // causing the diff to be computed against the wrong commit. | ||
| String fetchResult = runGit(repoPath, "fetch", "origin"); | ||
| if (fetchResult == null) { | ||
| log.warn("Git fetch failed before diff computation: repoId={}, headSha={}", repositoryId, headSha); | ||
| } else { | ||
| log.debug("Fetched latest refs before diff computation: repoId={}", repositoryId); | ||
| } | ||
|
|
||
| try { | ||
| String[] range = resolveDiffRange(repoPath, targetBranch, sourceBranch, headSha); |
There was a problem hiding this comment.
git fetch origin is executed via the system git CLI, but the local clones are created/fetched via JGit with a CredentialsProvider (the token is not persisted into .git/config). For private repos, the CLI fetch is likely to fail due to missing credentials, which would leave refs stale and defeat the purpose of this change. Consider fetching through GitRepositoryManager/JGit (or otherwise ensuring non-interactive credentials are available for the CLI fetch) and treat a fetch failure as a hard fallback to SHA-only strategies that don't rely on updated remote refs.
| // Fetch latest refs from remote before computing the diff. | |
| // Without this, the local clone may have stale branch refs from an earlier push, | |
| // causing the diff to be computed against the wrong commit. | |
| String fetchResult = runGit(repoPath, "fetch", "origin"); | |
| if (fetchResult == null) { | |
| log.warn("Git fetch failed before diff computation: repoId={}, headSha={}", repositoryId, headSha); | |
| } else { | |
| log.debug("Fetched latest refs before diff computation: repoId={}", repositoryId); | |
| } | |
| try { | |
| String[] range = resolveDiffRange(repoPath, targetBranch, sourceBranch, headSha); | |
| // Do not refresh refs via the system git CLI here. Repository access is managed | |
| // through JGit credentials, which are not guaranteed to be available to an | |
| // external `git fetch origin` process for private repositories. If we cannot | |
| // guarantee fresh remote refs, we must avoid branch-based diff strategies that | |
| // rely on them and fall back to SHA-based resolution instead. | |
| log.debug( | |
| "Skipping CLI fetch before diff computation and forcing SHA-based fallback: repoId={}, headSha={}, sourceBranch={}", | |
| repositoryId, | |
| headSha, | |
| sourceBranch | |
| ); | |
| try { | |
| String[] range = resolveDiffRange(repoPath, targetBranch, null, headSha); |
| if (line.startsWith("+") && !line.startsWith("+++")) addedLines++; | ||
| else if (line.startsWith("-") && !line.startsWith("---")) removedLines++; |
There was a problem hiding this comment.
The added/removed line counting logic will undercount when the actual code line begins with ++ or -- because added lines like ++i; become +++i; and removed lines like --i; become ---i;, which are currently excluded by the startsWith("+++") / startsWith("---") checks. To avoid misclassification, only exclude the diff header lines ("+++ " / "--- " with a following space, or "+++\t"/"---\t"), not any content line that happens to start with those sequences.
| if (line.startsWith("+") && !line.startsWith("+++")) addedLines++; | |
| else if (line.startsWith("-") && !line.startsWith("---")) removedLines++; | |
| boolean isAddedFileHeader = line.startsWith("+++ ") || line.startsWith("+++\t"); | |
| boolean isRemovedFileHeader = line.startsWith("--- ") || line.startsWith("---\t"); | |
| if (line.startsWith("+") && !isAddedFileHeader) addedLines++; | |
| else if (line.startsWith("-") && !isRemovedFileHeader) removedLines++; |
| for (String line : diff.split("\n", -1)) { | ||
| if (line.startsWith("+") && !line.startsWith("+++")) addedLines++; | ||
| else if (line.startsWith("-") && !line.startsWith("---")) removedLines++; |
There was a problem hiding this comment.
diff.split("\n", -1) builds a full array using regex splitting, which can add significant CPU/memory overhead for large diffs (this method already holds the entire diff string). Since this is only for metrics, consider iterating over the string with an index / BufferedReader(new StringReader(diff)) to count line prefixes without allocating an extra array.
| for (String line : diff.split("\n", -1)) { | |
| if (line.startsWith("+") && !line.startsWith("+++")) addedLines++; | |
| else if (line.startsWith("-") && !line.startsWith("---")) removedLines++; | |
| int lineStart = 0; | |
| while (lineStart <= diff.length()) { | |
| int lineEnd = diff.indexOf('\n', lineStart); | |
| if (lineEnd == -1) { | |
| lineEnd = diff.length(); | |
| } | |
| if (lineStart < lineEnd) { | |
| char firstChar = diff.charAt(lineStart); | |
| if (firstChar == '+') { | |
| if (lineEnd - lineStart < 3 || diff.charAt(lineStart + 1) != '+' || diff.charAt(lineStart + 2) != '+') { | |
| addedLines++; | |
| } | |
| } else if (firstChar == '-') { | |
| if (lineEnd - lineStart < 3 || diff.charAt(lineStart + 1) != '-' || diff.charAt(lineStart + 2) != '-') { | |
| removedLines++; | |
| } | |
| } | |
| } | |
| if (lineEnd == diff.length()) { | |
| break; | |
| } | |
| lineStart = lineEnd + 1; |
| // Strategy 1: Branch-based diff (works if source branch still exists and is current) | ||
| String branchBase = "origin/" + targetBranch; | ||
| String branchHead = "origin/" + sourceBranch; | ||
| String statCheck = runGit(repoPath, "diff", "--stat", branchBase + ".." + branchHead); | ||
| if (statCheck != null && !statCheck.isBlank()) { |
There was a problem hiding this comment.
resolveDiffRange is called from multiple places (diff precompute, diff scope filtering, diff-hunk validation). Only computeAndStoreDiff now does a fetch, so other call sites can still operate on stale origin/* refs or even miss the headSha object if the checkout wasn't recently fetched (e.g., deliver runs in a different process or long after job preparation). Consider moving the ref update into a shared helper (or into resolveDiffRange itself) so all diff computations consistently work against up-to-date refs.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
server/application-server/src/main/java/de/tum/in/www1/hephaestus/agent/handler/PullRequestReviewHandler.java (1)
620-647:⚠️ Potential issue | 🟠 MajorReuse this resolved range instead of recomputing it later.
This method now fetches and resolves a fresh range once, but
addCommitLog,computeDiffStatFiles, andcomputeDiffValidLinesall callresolveDiffRange(...)again against live repo state. If refs move between preparation and delivery, the agent reviews one diff while commit metadata, scope filtering, or line correction use another. Persistbase/head(and ideally the exact strategy) and reuse that everywhere downstream.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/application-server/src/main/java/de/tum/in/www1/hephaestus/agent/handler/PullRequestReviewHandler.java` around lines 620 - 647, The code currently calls resolveDiffRange(...) once but later re-invokes it in addCommitLog, computeDiffStatFiles, and computeDiffValidLines which can produce mismatched base/head if refs move; change the flow to capture and persist the resolved range (base and head SHAs) and the chosen strategy (e.g., the existing range array and strategyUsed) after the initial resolve and pass these canonical values into addCommitLog, computeDiffStatFiles, and computeDiffValidLines (or store them as fields on the current PullRequestReviewHandler instance used for this run) so all downstream computations use the same base/head and strategy instead of calling resolveDiffRange(...) again.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@server/application-server/src/main/java/de/tum/in/www1/hephaestus/agent/handler/PullRequestReviewHandler.java`:
- Around line 612-617: The fetchResult check in PullRequestReviewHandler
currently treats a failed runGit(repoPath, "fetch", "origin") as a warning but
still allows resolveDiffRange to potentially return cached ref names like
origin/<target>..origin/<source>, which lets stale target refs slip through
under Strategy 1; change the logic so that when fetchResult == null you mark
refs as untrusted (e.g. set a boolean like refsTrusted=false) and either force
resolveDiffRange to fall back to SHA-only diff ranges (use headSha and commit
SHAs directly) or skip any precomputation that relies on remote ref names;
update the call sites that invoke resolveDiffRange (and any precompute paths) to
respect this flag so only SHA-only resolution is used when fetch failed.
---
Outside diff comments:
In
`@server/application-server/src/main/java/de/tum/in/www1/hephaestus/agent/handler/PullRequestReviewHandler.java`:
- Around line 620-647: The code currently calls resolveDiffRange(...) once but
later re-invokes it in addCommitLog, computeDiffStatFiles, and
computeDiffValidLines which can produce mismatched base/head if refs move;
change the flow to capture and persist the resolved range (base and head SHAs)
and the chosen strategy (e.g., the existing range array and strategyUsed) after
the initial resolve and pass these canonical values into addCommitLog,
computeDiffStatFiles, and computeDiffValidLines (or store them as fields on the
current PullRequestReviewHandler instance used for this run) so all downstream
computations use the same base/head and strategy instead of calling
resolveDiffRange(...) again.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 298970e9-a48f-4f39-990c-f8024aa14e51
📒 Files selected for processing (1)
server/application-server/src/main/java/de/tum/in/www1/hephaestus/agent/handler/PullRequestReviewHandler.java
| String fetchResult = runGit(repoPath, "fetch", "origin"); | ||
| if (fetchResult == null) { | ||
| log.warn("Git fetch failed before diff computation: repoId={}, headSha={}", repositoryId, headSha); | ||
| } else { | ||
| log.debug("Fetched latest refs before diff computation: repoId={}", repositoryId); | ||
| } |
There was a problem hiding this comment.
Treat a failed fetch as “refs are untrusted.”
If git fetch origin fails here, resolveDiffRange can still return origin/<target>..origin/<source> from cached local refs. Because Strategy 1 only validates the source ref against headSha, a stale origin/<targetBranch> can still slip through and recreate the wrong diff. Please force SHA-only fallback, or skip precomputation, when the refresh fails.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@server/application-server/src/main/java/de/tum/in/www1/hephaestus/agent/handler/PullRequestReviewHandler.java`
around lines 612 - 617, The fetchResult check in PullRequestReviewHandler
currently treats a failed runGit(repoPath, "fetch", "origin") as a warning but
still allows resolveDiffRange to potentially return cached ref names like
origin/<target>..origin/<source>, which lets stale target refs slip through
under Strategy 1; change the logic so that when fetchResult == null you mark
refs as untrusted (e.g. set a boolean like refsTrusted=false) and either force
resolveDiffRange to fall back to SHA-only diff ranges (use headSha and commit
SHAs directly) or skip any precomputation that relies on remote ref names;
update the call sites that invoke resolveDiffRange (and any precompute paths) to
respect this flag so only SHA-only resolution is used when fetch failed.
…ce review The initial fix used raw `git fetch origin` which lacks authentication credentials and would fail silently on private repositories (the remote URL in the clone config has no embedded token). The agent would then proceed with the stale clone and produce wrong diffs. This replaces the unauthenticated fetch with a proper call to GitRepositoryManager.ensureRepository() using the workspace's GitLab PAT via GitLabTokenService. The fetch: - Authenticates via JGit CredentialsProvider (same as push webhooks) - Acquires a write lock to coordinate with concurrent push handlers - Falls back gracefully if the token is unavailable (e.g. GitHub workspaces), relying on the headSha validation in resolveDiffRange The headSha validation in Strategy 1 remains as defense-in-depth: even if a fetch fails, the code will detect stale branch refs and fall through to SHA-based diff strategies.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
server/application-server/src/main/java/de/tum/in/www1/hephaestus/agent/handler/PullRequestReviewHandler.java (1)
177-190:⚠️ Potential issue | 🟠 MajorFetch before building metadata-derived commit history.
Line 185 calls
storeMetadataAndComments(), which invokesaddCommitLog()and thereforeresolveDiffRange(), before Line 190 reaches the new fetch path. That leaves.context/metadata.jsonfree to carry a stale commit list even when the diff itself is refreshed, so the agent can still review the wrong MR history. Please move the fetch ahead of metadata generation, or makeaddCommitLog()consume the same already-refreshed range.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/application-server/src/main/java/de/tum/in/www1/hephaestus/agent/handler/PullRequestReviewHandler.java` around lines 177 - 190, The metadata is being built from potentially stale commit history because storeMetadataAndComments (which calls addCommitLog and resolveDiffRange) runs before the repo fetch/diff refresh; move the fetch/diff step earlier or make addCommitLog use the refreshed range. Concretely: call ensureRepositoryAvailable and computeAndStoreDiff(repositoryId, metadata, job) before storeMetadataAndComments(files, pullRequest, pullRequestId, metadata) and storeContributorHistory(files, pullRequest, job), or alternatively change addCommitLog/resolveDiffRange to accept and use the already-refreshed diff range produced by computeAndStoreDiff so metadata.json is written from the same refreshed commit range.
♻️ Duplicate comments (1)
server/application-server/src/main/java/de/tum/in/www1/hephaestus/agent/handler/PullRequestReviewHandler.java (1)
528-583:⚠️ Potential issue | 🟠 MajorDon’t trust branch refs when the refresh was skipped or failed.
If
fetchBeforeDiff()returns early or hits the warning path at Lines 577-581,resolveDiffRange()can still returnorigin/<target>..origin/<source>as long as the source ref matchesheadSha. A staleorigin/<targetBranch>is enough to recreate the wrong diff, so the stale-diff fix is still incomplete. Please propagate a “refs trusted” signal from the fetch step and disable Strategy 1 unless the refresh succeeded, or validate the target ref as well before returning the branch-based range.Also applies to: 999-1020
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/application-server/src/main/java/de/tum/in/www1/hephaestus/agent/handler/PullRequestReviewHandler.java` around lines 528 - 583, fetchBeforeDiff currently may skip or fail but leaves branch refs trusted; change it to explicitly return a boolean (or set a job-scoped "refsTrusted" flag) indicating whether the authenticated fetch completed successfully, and set that flag to false on any early return or exception in fetchBeforeDiff; then update resolveDiffRange to consult that flag and refuse to use Strategy 1 (the origin/<target>..origin/<source> branch-based range) unless refsTrusted is true — alternatively, if you must allow branch-based ranges when fetch failed, add an explicit validation step in resolveDiffRange that resolves the origin/<target> ref and verifies it matches the expected target commit before accepting Strategy 1. Ensure references to fetchBeforeDiff, resolveDiffRange, Strategy 1, and the job metadata/flag are used so reviewers can locate the changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In
`@server/application-server/src/main/java/de/tum/in/www1/hephaestus/agent/handler/PullRequestReviewHandler.java`:
- Around line 177-190: The metadata is being built from potentially stale commit
history because storeMetadataAndComments (which calls addCommitLog and
resolveDiffRange) runs before the repo fetch/diff refresh; move the fetch/diff
step earlier or make addCommitLog use the refreshed range. Concretely: call
ensureRepositoryAvailable and computeAndStoreDiff(repositoryId, metadata, job)
before storeMetadataAndComments(files, pullRequest, pullRequestId, metadata) and
storeContributorHistory(files, pullRequest, job), or alternatively change
addCommitLog/resolveDiffRange to accept and use the already-refreshed diff range
produced by computeAndStoreDiff so metadata.json is written from the same
refreshed commit range.
---
Duplicate comments:
In
`@server/application-server/src/main/java/de/tum/in/www1/hephaestus/agent/handler/PullRequestReviewHandler.java`:
- Around line 528-583: fetchBeforeDiff currently may skip or fail but leaves
branch refs trusted; change it to explicitly return a boolean (or set a
job-scoped "refsTrusted" flag) indicating whether the authenticated fetch
completed successfully, and set that flag to false on any early return or
exception in fetchBeforeDiff; then update resolveDiffRange to consult that flag
and refuse to use Strategy 1 (the origin/<target>..origin/<source> branch-based
range) unless refsTrusted is true — alternatively, if you must allow
branch-based ranges when fetch failed, add an explicit validation step in
resolveDiffRange that resolves the origin/<target> ref and verifies it matches
the expected target commit before accepting Strategy 1. Ensure references to
fetchBeforeDiff, resolveDiffRange, Strategy 1, and the job metadata/flag are
used so reviewers can locate the changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 53d8128d-f1b4-443a-b2a5-c91c84d70c4d
📒 Files selected for processing (1)
server/application-server/src/main/java/de/tum/in/www1/hephaestus/agent/handler/PullRequestReviewHandler.java
…call sites Update JobTypeHandlerConfiguration, PullRequestReviewHandlerTest, and JobTypeHandlerRegistryTest to pass the new optional GitLabTokenService parameter (null in tests).
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@server/application-server/src/test/java/de/tum/in/www1/hephaestus/agent/handler/PullRequestReviewHandlerTest.java`:
- Around line 94-95: Add a new unit test in PullRequestReviewHandlerTest that
constructs PullRequestReviewHandler with a mocked GitLabTokenService (instead of
null) and stubs getToken(...) to return a non-empty token; invoke the handler
method under test (e.g., handlePullRequestReview or whichever entrypoint is used
in this test class) and assert that the code path which performs an
authenticated fetch is executed by verifying interactions with the downstream
collaborator used for authenticated requests (e.g., a mocked HTTP client or the
component that consumes the token) and/or verifying that
GitLabTokenService.getToken(...) was called; ensure the test name documents it
covers the non-null GitLabTokenService authenticated-fetch branch.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e0dfaad7-2c5f-4f46-9ab9-869e0228d2f8
📒 Files selected for processing (3)
server/application-server/src/main/java/de/tum/in/www1/hephaestus/agent/handler/JobTypeHandlerConfiguration.javaserver/application-server/src/test/java/de/tum/in/www1/hephaestus/agent/handler/JobTypeHandlerRegistryTest.javaserver/application-server/src/test/java/de/tum/in/www1/hephaestus/agent/handler/PullRequestReviewHandlerTest.java
| feedbackService, | ||
| null |
There was a problem hiding this comment.
Add a test for the non-null GitLabTokenService path.
Line 95 hardwires null in shared setup, so this suite only covers fallback behavior. Please add at least one focused test that constructs PullRequestReviewHandler with a mocked GitLabTokenService and verifies the authenticated-fetch branch is exercised.
As per coding guidelines, "Focus on risk: cover critical flows and edge cases first when writing tests".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@server/application-server/src/test/java/de/tum/in/www1/hephaestus/agent/handler/PullRequestReviewHandlerTest.java`
around lines 94 - 95, Add a new unit test in PullRequestReviewHandlerTest that
constructs PullRequestReviewHandler with a mocked GitLabTokenService (instead of
null) and stubs getToken(...) to return a non-empty token; invoke the handler
method under test (e.g., handlePullRequestReview or whichever entrypoint is used
in this test class) and assert that the code path which performs an
authenticated fetch is executed by verifying interactions with the downstream
collaborator used for authenticated requests (e.g., a mocked HTTP client or the
component that consumes the token) and/or verifying that
GitLabTokenService.getToken(...) was called; ensure the test name documents it
covers the non-null GitLabTokenService authenticated-fetch branch.
… max-params arch rule The architecture test enforces a maximum of 6 parameters per method. Moving the optional GitLabTokenService from a bean method parameter to a class-level field keeps pullRequestReviewHandler at 6 parameters.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
server/application-server/src/main/java/de/tum/in/www1/hephaestus/agent/handler/PullRequestReviewHandler.java (1)
672-679:⚠️ Potential issue | 🟠 MajorTreat fetch skip/failure as “refs untrusted” before allowing branch-based range.
fetchBeforeDiff(Line 675) can return without refreshing refs (no token / failure), butresolveDiffRangecan still acceptorigin/<target>..origin/<source>if only the source tip matchesheadSha. A stale cachedorigin/<targetBranch>can still yield an incorrect diff range.Suggested fix
- private void fetchBeforeDiff(long repositoryId, AgentJob job) { + private boolean fetchBeforeDiff(long repositoryId, AgentJob job) { try { var workspace = job.getWorkspace(); if (workspace == null) { log.debug("No workspace on job, skipping pre-diff fetch: jobId={}", job.getId()); - return; + return false; } ... if (serverUrl == null || token == null) { log.debug("No token available for pre-diff fetch, relying on existing clone: repoId={}", repositoryId); - return; + return false; } ... gitRepositoryManager.ensureRepository(repositoryId, cloneUrl, token); log.debug("Fetched latest refs before diff computation: repoId={}, scopeId={}", repositoryId, scopeId); + return true; } catch (Exception e) { log.warn( "Pre-diff fetch failed (will proceed with existing clone): repoId={}, error={}", repositoryId, e.getMessage() ); + return false; } } - String[] range = resolveDiffRange(repoPath, targetBranch, sourceBranch, headSha); + boolean refsTrusted = fetchBeforeDiff(repositoryId, job); + String[] range = resolveDiffRange(repoPath, targetBranch, sourceBranch, headSha, refsTrusted); - private String[] resolveDiffRange(Path repoPath, String targetBranch, String sourceBranch, String headSha) { + private String[] resolveDiffRange( + Path repoPath, String targetBranch, String sourceBranch, String headSha, boolean refsTrusted + ) { - String statCheck = runGit(repoPath, "diff", "--stat", branchBase + ".." + branchHead); - if (statCheck != null && !statCheck.isBlank()) { + String statCheck = refsTrusted ? runGit(repoPath, "diff", "--stat", branchBase + ".." + branchHead) : null; + if (refsTrusted && statCheck != null && !statCheck.isBlank()) { ... }Also applies to: 999-1021
🧹 Nitpick comments (1)
server/application-server/src/main/java/de/tum/in/www1/hephaestus/agent/handler/PullRequestReviewHandler.java (1)
577-581: Add job/workspace context to this warning log.This warning is useful but hard to correlate without
jobId/scopeIdin the message fields.As per coding guidelines, "Include context in logs such as workspace, user, and request ID".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/application-server/src/main/java/de/tum/in/www1/hephaestus/agent/handler/PullRequestReviewHandler.java` around lines 577 - 581, The warning log in PullRequestReviewHandler currently calls log.warn("Pre-diff fetch failed (will proceed with existing clone): repoId={}, error={}", repositoryId, e.getMessage()); update this log to include job/workspace context by adding the relevant identifiers (e.g., jobId and scopeId or workspaceId variables available in the method) into the message and argument list (e.g., "Pre-diff fetch failed (will proceed with existing clone): repoId={}, jobId={}, scopeId={}, error={}") and pass repositoryId, jobId, scopeId, and e.getMessage() to log.warn so the warning contains the required context for correlation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@server/application-server/src/main/java/de/tum/in/www1/hephaestus/agent/handler/PullRequestReviewHandler.java`:
- Around line 577-581: The warning log in PullRequestReviewHandler currently
calls log.warn("Pre-diff fetch failed (will proceed with existing clone):
repoId={}, error={}", repositoryId, e.getMessage()); update this log to include
job/workspace context by adding the relevant identifiers (e.g., jobId and
scopeId or workspaceId variables available in the method) into the message and
argument list (e.g., "Pre-diff fetch failed (will proceed with existing clone):
repoId={}, jobId={}, scopeId={}, error={}") and pass repositoryId, jobId,
scopeId, and e.getMessage() to log.warn so the warning contains the required
context for correlation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 92fd7b82-14ba-4c71-9182-309a84cdbba8
📒 Files selected for processing (2)
server/application-server/src/main/java/de/tum/in/www1/hephaestus/agent/handler/JobTypeHandlerConfiguration.javaserver/application-server/src/main/java/de/tum/in/www1/hephaestus/agent/handler/PullRequestReviewHandler.java
✅ Files skipped from review due to trivial changes (1)
- server/application-server/src/main/java/de/tum/in/www1/hephaestus/agent/handler/JobTypeHandlerConfiguration.java
📚 Documentation Preview
|
|
🎉 This PR is included in version 0.56.5 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
Summary
Fixes a bug where the practice review pipeline computes a diff against a stale commit when a student pushes multiple commits to an MR branch in quick succession. This caused a production incident where a student received a review saying "no new code to review" despite adding 200 lines of SwiftUI code.
GitRepositoryManager.ensureRepository()with the workspace's GitLab PAT (viaGitLabTokenService) before computing the diff, ensuring the local clone has up-to-date refs. Falls back gracefully for GitHub workspaces or if the token is unavailable.resolveDiffRangenow verifies thatorigin/{sourceBranch}actually points to the expectedheadShabefore trusting it; falls through to SHA-based strategies (2 and 3) if stale.Root Cause
ensureRepositoryAvailableonly checks clone existence — never fetchesresolveDiffRangeStrategy 1 usesorigin/{sourceBranch}without verifying it matchesheadShafrom the webhookNOT_APPLICABLEfindings on an MR with real codeTest plan
mvnw compile)"Stale branch ref detected"warnings and diff quality metrics (+N/-N lines)Summary by CodeRabbit