Skip to content

fix(server): prevent stale diff in practice review pipeline #981

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
FelixTJDietrich merged 6 commits into from
Apr 10, 2026
Merged

fix(server): prevent stale diff in practice review pipeline #981

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
d1692a6
fix(server): prevent stale diff in practice review pipeline
FelixTJDietrich Apr 10, 2026
f5f8bbc
style: format PullRequestReviewHandler with Prettier
FelixTJDietrich Apr 10, 2026
d44e55a
fix(server): use authenticated fetch to prevent stale diffs in practi…
FelixTJDietrich Apr 10, 2026
4eb41cb
fix(server): pass GitLabTokenService to all PullRequestReviewHandler …
FelixTJDietrich Apr 10, 2026
108b8b9
fix(server): move GitLabTokenService to config class field to satisfy…
FelixTJDietrich Apr 10, 2026
923c86e
style(server): replace inline FQNs with proper imports
FelixTJDietrich Apr 10, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 6 additions & 2 deletions ...er/src/main/java/de/tum/in/www1/hephaestus/agent/handler/JobTypeHandlerConfiguration.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,10 @@ public JobTypeHandler pullRequestReviewHandler(
PracticeRepository practiceRepository,
PracticeDetectionResultParser resultParser,
PracticeDetectionDeliveryService deliveryService,
FeedbackDeliveryService feedbackService
FeedbackDeliveryService feedbackService,
@org.springframework.beans.factory.annotation.Autowired(
required = false
) @org.springframework.lang.Nullable de.tum.in.www1.hephaestus.gitprovider.common.gitlab.GitLabTokenService gitLabTokenService
) {
return new PullRequestReviewHandler(
objectMapper,
Expand All @@ -105,7 +108,8 @@ public JobTypeHandler pullRequestReviewHandler(
contributorHistoryProvider(),
resultParser,
deliveryService,
feedbackService
feedbackService,
gitLabTokenService
);
}

Expand Down
124 changes: 111 additions & 13 deletions ...erver/src/main/java/de/tum/in/www1/hephaestus/agent/handler/PullRequestReviewHandler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,9 @@ public class PullRequestReviewHandler implements JobTypeHandler {
private final PracticeDetectionDeliveryService deliveryService;
private final FeedbackDeliveryService feedbackService;

@Nullable
private final de.tum.in.www1.hephaestus.gitprovider.common.gitlab.GitLabTokenService gitLabTokenService;

PullRequestReviewHandler(
ObjectMapper objectMapper,
GitRepositoryManager gitRepositoryManager,
Expand All @@ -102,7 +105,10 @@ public class PullRequestReviewHandler implements JobTypeHandler {
ContributorHistoryProvider contributorHistoryProvider,
PracticeDetectionResultParser resultParser,
PracticeDetectionDeliveryService deliveryService,
FeedbackDeliveryService feedbackService
FeedbackDeliveryService feedbackService,
@org.springframework.beans.factory.annotation.Autowired(
required = false
) @Nullable de.tum.in.www1.hephaestus.gitprovider.common.gitlab.GitLabTokenService gitLabTokenService
) {
this.objectMapper = objectMapper;
this.gitRepositoryManager = gitRepositoryManager;
Expand All @@ -113,6 +119,7 @@ public class PullRequestReviewHandler implements JobTypeHandler {
this.resultParser = resultParser;
this.deliveryService = deliveryService;
this.feedbackService = feedbackService;
this.gitLabTokenService = gitLabTokenService;
}

@Override
Expand Down Expand Up @@ -167,8 +174,8 @@ public Map<String, byte[]> prepareInputFiles(AgentJob job) {

Map<String, byte[]> files = new HashMap<>();

// PR review requires a pre-prepared local checkout. The sandbox never clones or fetches
// repositories on demand; it only mounts an existing host-side checkout.
// PR review requires a local checkout (cloned by push webhook).
// The diff step will fetch to ensure refs are current before computing.
ensureRepositoryAvailable(repositoryId);

// Load PR entity once — shared by metadata, comments, and contributor history
Expand All @@ -180,7 +187,7 @@ public Map<String, byte[]> prepareInputFiles(AgentJob job) {

// Pre-compute diff: source branches may be deleted after merge,
// so we compute the diff from the merge commit graph using SHAs.
computeAndStoreDiff(files, repositoryId, metadata);
computeAndStoreDiff(files, repositoryId, metadata, job);

// Build a structured per-file diff summary optimized for single-pass AI consumption.
// This is structural transformation (splitting on "diff --git" boundaries), not judgment.
Expand Down Expand Up @@ -500,11 +507,10 @@ public void deliver(AgentJob job) {
// -------------------------------------------------------------------------

/**
* Require a pre-prepared local repository checkout for bind-mounting.
* Require a local repository checkout for bind-mounting.
*
* <p>This review flow never clones or fetches repositories on demand. Repository preparation
* must happen ahead of time via the normal sync/bootstrap path so sandbox runs stay offline and
* deterministic with respect to repo contents.
* <p>The repository must have been cloned by a prior push webhook. The diff computation
* step ({@link #fetchBeforeDiff}) may fetch to update refs before computing the diff.
*/
private void ensureRepositoryAvailable(long repositoryId) {
if (!gitRepositoryManager.isEnabled()) {
Expand All @@ -519,6 +525,63 @@ private void ensureRepositoryAvailable(long repositoryId) {
}
}

/**
* Fetch latest refs from the remote before computing the diff.
* Uses GitLabTokenService for GitLab workspaces to get an authenticated fetch.
* Falls back gracefully on failure — the diff computation will still attempt
* with whatever refs are locally available.
*/
private void 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;
}

String serverUrl = null;
String token = null;
Long scopeId = workspace.getId();

if (gitLabTokenService != null) {
try {
serverUrl = gitLabTokenService.resolveServerUrl(scopeId);
token = gitLabTokenService.getAccessToken(scopeId);
} catch (Exception e) {
log.debug("GitLab token not available for fetch: scopeId={}, reason={}", scopeId, e.getMessage());
}
}

// For GitHub workspaces or when GitLab token is unavailable, the repo may already
// be current (fetched by push webhook). The headSha validation in resolveDiffRange
// will catch staleness even without a fetch here.
if (serverUrl == null || token == null) {
log.debug("No token available for pre-diff fetch, relying on existing clone: repoId={}", repositoryId);
return;
}

JsonNode metadata = job.getMetadata();
String repoFullName =
metadata != null && metadata.has("repository_full_name")
? metadata.get("repository_full_name").asText()
: null;
if (repoFullName == null) {
log.debug("No repository_full_name in metadata, skipping pre-diff fetch");
return;
}

String cloneUrl = serverUrl + "/" + repoFullName + ".git";
gitRepositoryManager.ensureRepository(repositoryId, cloneUrl, token);
log.debug("Fetched latest refs before diff computation: repoId={}, scopeId={}", repositoryId, scopeId);
} catch (Exception e) {
log.warn(
"Pre-diff fetch failed (will proceed with existing clone): repoId={}, error={}",
repositoryId,
e.getMessage()
);
}
}

/** Build and store pull request metadata and review comments as context JSON files. */
private void storeMetadataAndComments(
Map<String, byte[]> files,
Expand Down Expand Up @@ -596,7 +659,7 @@ private void storeContributorHistory(Map<String, byte[]> files, PullRequest pull
* between merge^1 (target before merge) and merge^2 (MR tip). Falls back to
* target_branch..head_sha if branch still exists.
*/
private void computeAndStoreDiff(Map<String, byte[]> files, long repositoryId, JsonNode metadata) {
private void computeAndStoreDiff(Map<String, byte[]> files, long repositoryId, JsonNode metadata, AgentJob job) {
String headSha = metadata.has("commit_sha") ? metadata.get("commit_sha").asText() : null;
String targetBranch = requireText(metadata, "target_branch");
String sourceBranch = requireText(metadata, "source_branch");
Expand All @@ -605,6 +668,12 @@ private void computeAndStoreDiff(Map<String, byte[]> files, long repositoryId, J
return;
}
Path repoPath = gitRepositoryManager.getRepositoryPath(repositoryId);

// 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 (see: stale-diff incident).
fetchBeforeDiff(repositoryId, job);

try {
String[] range = resolveDiffRange(repoPath, targetBranch, sourceBranch, headSha);
Comment on lines +672 to 678

Copilot AI Apr 10, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
// 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);

Copilot uses AI. Check for mistakes.
if (range == null) {
Expand All @@ -622,11 +691,24 @@ private void computeAndStoreDiff(Map<String, byte[]> files, long repositoryId, J
if (diffStat != null) {
files.put(".context/diff_stat.txt", diffStat.getBytes(StandardCharsets.UTF_8));
}

// Log diff quality metrics for stale-diff incident detection
int addedLines = 0;
int removedLines = 0;
for (String line : diff.split("\n", -1)) {
if (line.startsWith("+") && !line.startsWith("+++")) addedLines++;
else if (line.startsWith("-") && !line.startsWith("---")) removedLines++;
Comment on lines +699 to +700

Copilot AI Apr 10, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
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++;

Copilot uses AI. Check for mistakes.
Comment on lines +698 to +700

Copilot AI Apr 10, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
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;

Copilot uses AI. Check for mistakes.
}
String strategyUsed = range[1].equals(headSha) ? "SHA-based" : "branch-based";
log.info(
"Pre-computed diff: {} bytes (annotated: {} bytes), diffStat={} bytes, headSha={}",
"Pre-computed diff: strategy={}, range={}..{}, +{}/-{} lines, {} bytes (annotated: {} bytes), headSha={}",
strategyUsed,
range[0],
range[1],
addedLines,
removedLines,
diff.length(),
annotatedDiff.length(),
diffStat != null ? diffStat.length() : 0,
headSha
);
} else {
Expand Down Expand Up @@ -914,12 +996,28 @@ private static boolean isInternalContextPath(String path) {
*/
@Nullable
private String[] resolveDiffRange(Path repoPath, String targetBranch, String sourceBranch, String headSha) {
// Strategy 1: Branch-based diff (works if source branch still exists)
// 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()) {
Comment on lines +999 to 1003

Copilot AI Apr 10, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.
return new String[] { branchBase, branchHead };
// Verify the local branch ref matches the expected head commit.
// If the local clone has a stale ref from an earlier push, skip to SHA-based strategies.
String branchTip = runGit(repoPath, "rev-parse", branchHead);
if (
branchTip != null &&
headSha != null &&
branchTip.trim().startsWith(headSha.substring(0, Math.min(headSha.length(), 12)))
) {
return new String[] { branchBase, branchHead };
}
log.warn(
"Stale branch ref detected: branch={}, expected={}, actual={}",
branchHead,
headSha,
branchTip != null ? branchTip.trim() : "null"
);
// Fall through to SHA-based strategies
}

// Strategy 2: Find merge commit that has headSha as second parent
Expand Down
3 changes: 2 additions & 1 deletion ...ver/src/test/java/de/tum/in/www1/hephaestus/agent/handler/JobTypeHandlerRegistryTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,8 @@ private JobTypeHandler prReviewHandler() {
contributorHistoryProvider,
parser,
deliveryService,
feedbackService
feedbackService,
null
);
}

Expand Down
3 changes: 2 additions & 1 deletion ...r/src/test/java/de/tum/in/www1/hephaestus/agent/handler/PullRequestReviewHandlerTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,8 @@ void setUp() {
contributorHistoryProvider,
resultParser,
deliveryService,
feedbackService
feedbackService,
null
Comment on lines +94 to +95

@coderabbitai coderabbitai Bot Apr 10, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

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.

);
}

Expand Down
Loading