Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,9 @@
import de.tum.in.www1.hephaestus.gitprovider.pullrequest.PullRequestRepository;
import de.tum.in.www1.hephaestus.gitprovider.pullrequestreviewcomment.PullRequestReviewCommentRepository;
import de.tum.in.www1.hephaestus.practices.PracticeRepository;
import de.tum.in.www1.hephaestus.practices.finding.ContributorHistoryProvider;
import de.tum.in.www1.hephaestus.practices.finding.PracticeDetectionProperties;
import de.tum.in.www1.hephaestus.practices.finding.PracticeFindingRepository;
import de.tum.in.www1.hephaestus.practices.review.PracticeReviewDeliveryGate;
import de.tum.in.www1.hephaestus.practices.review.PracticeReviewProperties;
import de.tum.in.www1.hephaestus.workspace.WorkspaceRepository;
Expand All @@ -33,17 +35,20 @@ public class JobTypeHandlerConfiguration {

private final ObjectMapper objectMapper;
private final GitRepositoryManager gitRepositoryManager;
private final PracticeFindingRepository practiceFindingRepository;
private final PracticeReviewDeliveryGate deliveryGate;
private final PracticeReviewProperties reviewProperties;

JobTypeHandlerConfiguration(
ObjectMapper objectMapper,
GitRepositoryManager gitRepositoryManager,
PracticeFindingRepository practiceFindingRepository,
PracticeReviewDeliveryGate deliveryGate,
PracticeReviewProperties reviewProperties
) {
this.objectMapper = objectMapper;
this.gitRepositoryManager = gitRepositoryManager;
this.practiceFindingRepository = practiceFindingRepository;
this.deliveryGate = deliveryGate;
this.reviewProperties = reviewProperties;
}
Expand All @@ -53,6 +58,11 @@ public PracticeDetectionResultParser practiceDetectionResultParser(PracticeDetec
return new PracticeDetectionResultParser(objectMapper, properties.maxFindingsPerJob());
}

@Bean
ContributorHistoryProvider contributorHistoryProvider() {
return new ContributorHistoryProvider(practiceFindingRepository, objectMapper);
}

@Bean
PullRequestCommentPoster pullRequestCommentPoster(
GitHubGraphQlClientProvider gitHubProvider,
Expand Down Expand Up @@ -106,6 +116,9 @@ public JobTypeHandler pullRequestReviewHandler(
pullRequestRepository,
reviewCommentRepository,
practiceRepository,
// CGLIB proxy call: returns the singleton bean. Not a @Bean parameter to stay
// within the architecture test limit of 6 parameters per @Bean method.
contributorHistoryProvider(),
resultParser,
deliveryService,
feedbackService
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,11 +17,13 @@
import de.tum.in.www1.hephaestus.gitprovider.pullrequest.PullRequestRepository;
import de.tum.in.www1.hephaestus.gitprovider.pullrequestreviewcomment.PullRequestReviewCommentRepository;
import de.tum.in.www1.hephaestus.practices.PracticeRepository;
import de.tum.in.www1.hephaestus.practices.finding.ContributorHistoryProvider;
import de.tum.in.www1.hephaestus.practices.model.Practice;
import java.nio.file.Path;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
import java.util.Optional;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

Expand All @@ -35,12 +37,13 @@
* <p>Container workspace layout:
* <pre>
* /workspace/
* ├── repo/ # Real git repo (read-only bind mount)
* ├── repo/ # Real git repo (read-only bind mount)
* ├── .context/
* │ ├── metadata.json # Title, body, author, branches, stats
* │ └── comments.json # Review comments (ordered by creation time)
* ├── .prompt # Written by executor from buildPrompt()
* └── .output/ # Agent writes results here
* │ ├── metadata.json # Title, body, author, branches, stats
* │ ├── comments.json # Review comments (ordered by creation time)
* │ └── contributor_history.json # Aggregated practice verdict history (optional)
* ├── .prompt # Written by executor from buildPrompt()
* └── .output/ # Agent writes results here
* </pre>
*/
public class PullRequestReviewHandler implements JobTypeHandler {
Expand All @@ -55,6 +58,7 @@ public class PullRequestReviewHandler implements JobTypeHandler {
private final PullRequestRepository pullRequestRepository;
private final PullRequestReviewCommentRepository reviewCommentRepository;
private final PracticeRepository practiceRepository;
private final ContributorHistoryProvider contributorHistoryProvider;
private final PracticeDetectionResultParser resultParser;
private final PracticeDetectionDeliveryService deliveryService;
private final FeedbackDeliveryService feedbackService;
Expand All @@ -65,6 +69,7 @@ public class PullRequestReviewHandler implements JobTypeHandler {
PullRequestRepository pullRequestRepository,
PullRequestReviewCommentRepository reviewCommentRepository,
PracticeRepository practiceRepository,
ContributorHistoryProvider contributorHistoryProvider,
PracticeDetectionResultParser resultParser,
PracticeDetectionDeliveryService deliveryService,
FeedbackDeliveryService feedbackService
Expand All @@ -74,6 +79,7 @@ public class PullRequestReviewHandler implements JobTypeHandler {
this.pullRequestRepository = pullRequestRepository;
this.reviewCommentRepository = reviewCommentRepository;
this.practiceRepository = practiceRepository;
this.contributorHistoryProvider = contributorHistoryProvider;
this.resultParser = resultParser;
this.deliveryService = deliveryService;
this.feedbackService = feedbackService;
Expand Down Expand Up @@ -134,8 +140,13 @@ public Map<String, byte[]> prepareInputFiles(AgentJob job) {
// Ensure repo is cloned/fetched before volumeMounts() resolves the path
ensureRepositoryCloned(metadata, repositoryId);

// Only inject DB-sourced context (metadata + comments)
storeMetadataAndComments(files, pullRequestId, metadata);
// Load PR entity once — shared by metadata, comments, and contributor history
PullRequest pullRequest = pullRequestRepository.findByIdWithAllForGate(pullRequestId).orElse(null);

// Inject DB-sourced context (metadata + comments + contributor history)
storeMetadataAndComments(files, pullRequest, pullRequestId, metadata);
storeContributorHistory(files, pullRequest, job);
verifyActivePractices(job);

long elapsedMs = (System.nanoTime() - startNanos) / 1_000_000;
log.info(
Expand Down Expand Up @@ -312,6 +323,13 @@ private void appendInstructions(StringBuilder sb, int practiceCount) {
sb.append("PR metadata and review comments are at `/workspace/.context/metadata.json` ");
sb.append("and `/workspace/.context/comments.json`.\n");
sb.append('\n');
sb.append("**Contributor history:** If `/workspace/.context/contributor_history.json` exists, ");
sb.append("it contains aggregated practice verdict counts from this contributor's prior PRs. ");
sb.append("Use this to calibrate your `guidanceMethod`: if a contributor has repeated NEGATIVE ");
sb.append("findings for a practice, escalate from MODELING to COACHING or SCAFFOLDING. ");
sb.append("If they show sustained POSITIVE verdicts, use REFLECTION or EXPLORATION. ");
sb.append("If the file is absent, this is a new contributor — default to COACHING.\n");

Copilot AI Mar 24, 2026

Copy link

Choose a reason for hiding this comment

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

The prompt implies that a missing /workspace/.context/contributor_history.json means “this is a new contributor”, but the file can also be absent due to DB/serialization failures or skipped enrichment. This can mislead guidanceMethod selection; consider wording it as “history unavailable” and defaulting to COACHING without asserting the contributor is new.

Suggested change
sb.append("If the file is absent, this is a new contributor — default to COACHING.\n");
sb.append("If the file is absent, contributor history is unavailable — default to COACHING.\n");

Copilot uses AI. Check for mistakes.
sb.append('\n');
sb.append("Write your final findings to `/workspace/.output/result.json`.\n");
sb.append('\n');
sb.append("**Review process:**\n");
Expand Down Expand Up @@ -555,8 +573,13 @@ private void ensureRepositoryCloned(JsonNode metadata, long repositoryId) {
}

/** Build and store pull request metadata and review comments as context JSON files. */
private void storeMetadataAndComments(Map<String, byte[]> files, long pullRequestId, JsonNode metadata) {
ObjectNode pullRequestMetadata = buildPullRequestMetadata(pullRequestId, metadata);
private void storeMetadataAndComments(
Map<String, byte[]> files,
PullRequest pullRequest,
long pullRequestId,
JsonNode metadata
) {
ObjectNode pullRequestMetadata = buildPullRequestMetadata(pullRequest, metadata);
try {
files.put(
".context/metadata.json",
Expand All @@ -577,11 +600,71 @@ private void storeMetadataAndComments(Map<String, byte[]> files, long pullReques
}
}

/**
* Build and store aggregated contributor practice history as a context JSON file.
*
* <p>Skips silently if the PR has no author or the contributor has no prior findings —
* the absence of the file signals a first-time contributor to the agent.
Comment on lines +606 to +607

Copilot AI Mar 24, 2026

Copy link

Choose a reason for hiding this comment

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

The Javadoc states that absence of contributor_history.json “signals a first-time contributor”, but in this implementation the file is also absent when the provider returns empty due to serialization errors or when the handler catches exceptions. Consider adjusting the comment so it doesn’t claim absence == new contributor.

Suggested change
* <p>Skips silently if the PR has no author or the contributor has no prior findings
* the absence of the file signals a first-time contributor to the agent.
* <p>Skips silently if the PR has no author or if no contributor history is available.
* In that case, {@code .context/contributor_history.json} is simply omitted and the agent
* should treat the absence as "no prior history is available" (which can include, but is
* not limited to, first-time contributors or failures while building the history).

Copilot uses AI. Check for mistakes.
*
* <p>Note: the contributor is always the PR author for PULL_REQUEST_REVIEW jobs.
* If a future job type evaluates reviewers, this lookup must change.
*/
private void storeContributorHistory(Map<String, byte[]> files, PullRequest pullRequest, AgentJob job) {
if (pullRequest == null || pullRequest.getAuthor() == null || job.getWorkspace() == null) {
if (pullRequest != null && pullRequest.getAuthor() == null) {
log.debug("Skipping contributor history: PR has no author, pullRequestId={}", pullRequest.getId());
}
return;
}
Long contributorId = pullRequest.getAuthor().getId();
Long workspaceId = job.getWorkspace().getId();

try {
Optional<byte[]> historyJson = contributorHistoryProvider.buildHistoryJson(contributorId, workspaceId);
historyJson.ifPresent(json -> {
files.put(".context/contributor_history.json", json);
log.info(
"Injected contributor history: {} bytes, contributorId={}, workspaceId={}",
json.length,
contributorId,
workspaceId
);
});
} catch (Exception e) {
log.warn(
"Failed to build contributor history, continuing without it: contributorId={}, workspaceId={}",
contributorId,
workspaceId,
e
);
}
}

/** Verify the job's workspace has active practices. */
private void verifyActivePractices(AgentJob job) {
if (job.getWorkspace() == null) {
throw new JobPreparationException("Job has no workspace: jobId=" + job.getId());
}
Long workspaceId = job.getWorkspace().getId();
List<Practice> practices = practiceRepository.findByWorkspaceIdAndActiveTrue(workspaceId);
if (practices.isEmpty()) {
throw new JobPreparationException(
"No active practices for workspace: workspaceId=" + workspaceId + ", jobId=" + job.getId()
);
}
log.info(
"Verified {} active practices for workspace: workspaceId={}, jobId={}",
practices.size(),
workspaceId,
job.getId()
);
}

// -------------------------------------------------------------------------
// Internal helpers
// -------------------------------------------------------------------------

private ObjectNode buildPullRequestMetadata(long pullRequestId, JsonNode jobMetadata) {
private ObjectNode buildPullRequestMetadata(PullRequest pullRequest, JsonNode jobMetadata) {
ObjectNode result = objectMapper.createObjectNode();

// Copy routing fields from job metadata (always present — validated in prepareInputFiles)
Expand All @@ -593,9 +676,8 @@ private ObjectNode buildPullRequestMetadata(long pullRequestId, JsonNode jobMeta
result.put("commit_sha", requireText(jobMetadata, "commit_sha"));

// Enrich from current DB state (title, body, author, etc.)
PullRequest pullRequest = pullRequestRepository.findByIdWithAllForGate(pullRequestId).orElse(null);
if (pullRequest == null) {
log.warn("Pull request not found in database during context preparation: pullRequestId={}", pullRequestId);
log.warn("Pull request not found in database during context preparation");

Copilot AI Mar 24, 2026

Copy link

Choose a reason for hiding this comment

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

This warn log lost the pullRequestId context (it’s no longer included in the message or MDC). Including the PR ID (and ideally workspace/repo) here would make diagnosing missing-PR enrichment much easier when scanning logs.

Suggested change
log.warn("Pull request not found in database during context preparation");
log.warn(
"Pull request not found in database during context preparation (pr_number={}, repository_full_name={})",
requireInt(jobMetadata, "pr_number"),
requireText(jobMetadata, "repository_full_name"));

Copilot uses AI. Check for mistakes.
result.put("enriched", false);
return result;
}
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,138 @@
package de.tum.in.www1.hephaestus.practices.finding;

import com.fasterxml.jackson.core.JsonProcessingException;
import com.fasterxml.jackson.databind.ObjectMapper;
import com.fasterxml.jackson.databind.node.ArrayNode;
import com.fasterxml.jackson.databind.node.ObjectNode;
import de.tum.in.www1.hephaestus.practices.model.Verdict;
import java.time.Instant;
import java.util.Comparator;
import java.util.LinkedHashMap;
import java.util.List;
import java.util.Map;
import java.util.Optional;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

/**
* Builds aggregated contributor practice history JSON for the review agent context.
*
* <p>Queries {@link PracticeFindingRepository} for verdict counts per practice and
* produces a compact JSON array suitable for injection into the agent sandbox as
* {@code .context/contributor_history.json}. NOT_APPLICABLE verdicts are excluded
* (they carry no calibration signal for guidance method selection).
*
* <p>Output is capped at {@value #MAX_PRACTICES} practices, sorted by NEGATIVE count
* descending so the most problematic practices are always included when truncation occurs.
*/
public class ContributorHistoryProvider {

private static final Logger log = LoggerFactory.getLogger(ContributorHistoryProvider.class);

/** Maximum number of practices included in the history JSON. */
static final int MAX_PRACTICES = 20;

private final PracticeFindingRepository practiceFindingRepository;
private final ObjectMapper objectMapper;

public ContributorHistoryProvider(PracticeFindingRepository practiceFindingRepository, ObjectMapper objectMapper) {
this.practiceFindingRepository = practiceFindingRepository;
this.objectMapper = objectMapper;
}

/**
* Builds contributor practice history JSON for agent context injection.
*
* @param contributorId the contributor whose history to aggregate
* @param workspaceId the workspace scope
* @return compact JSON bytes, or empty if the contributor has no relevant history
*/
public Optional<byte[]> buildHistoryJson(Long contributorId, Long workspaceId) {
List<ContributorPracticeSummary> summaries = practiceFindingRepository.findContributorPracticeSummary(
contributorId,
workspaceId
);

if (summaries.isEmpty()) {
return Optional.empty();
}

// Group by practice slug, accumulate verdict counts and track latest detection
Map<String, PracticeAggregate> byPractice = new LinkedHashMap<>();
for (ContributorPracticeSummary row : summaries) {
if (row.getVerdict() == Verdict.NOT_APPLICABLE) {
continue; // No calibration signal
}
byPractice.computeIfAbsent(row.getPracticeSlug(), slug -> new PracticeAggregate()).add(row);
}

if (byPractice.isEmpty()) {
return Optional.empty();
}

// Sort by NEGATIVE count desc, then slug for deterministic ordering
List<Map.Entry<String, PracticeAggregate>> sorted = byPractice
.entrySet()
.stream()
.sorted(
Comparator.<Map.Entry<String, PracticeAggregate>>comparingLong(e -> e.getValue().negative)
.reversed()
.thenComparing(Map.Entry::getKey)
)
.limit(MAX_PRACTICES)
.toList();

ArrayNode array = objectMapper.createArrayNode();
for (Map.Entry<String, PracticeAggregate> entry : sorted) {
ObjectNode node = objectMapper.createObjectNode();
PracticeAggregate agg = entry.getValue();
node.put("practice", entry.getKey());
node.put("positive", agg.positive);
node.put("negative", agg.negative);
node.put("needsReview", agg.needsReview);
node.put("lastSeen", agg.lastDetectedAt.toString());
array.add(node);
}

try {
byte[] json = objectMapper.writeValueAsBytes(array);
log.debug(
"Built contributor history: {} practices, {} bytes, contributorId={}, workspaceId={}",
sorted.size(),
json.length,
contributorId,
workspaceId
);
return Optional.of(json);
} catch (JsonProcessingException e) {
// Should never happen for ObjectNode serialization
log.error("Failed to serialize contributor history JSON", e);

Copilot AI Mar 24, 2026

Copy link

Choose a reason for hiding this comment

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

On serialization failure, this error log doesn’t include contributorId/workspaceId, and the caller will silently treat the history as absent. Add contextual identifiers to the log message (and optionally the practice count/byte size) so operators can correlate failures to specific jobs/users.

Suggested change
log.error("Failed to serialize contributor history JSON", e);
log.error(
"Failed to serialize contributor history JSON for contributorId={}, workspaceId={}, practices={}",
contributorId,
workspaceId,
sorted.size(),
e
);

Copilot uses AI. Check for mistakes.
return Optional.empty();
}
}

/**
* Mutable accumulator for per-practice verdict counts during aggregation.
*/
private static final class PracticeAggregate {

long positive;
long negative;
long needsReview;
Instant lastDetectedAt;

void add(ContributorPracticeSummary row) {
switch (row.getVerdict()) {
case POSITIVE -> positive += row.getCount();
case NEGATIVE -> negative += row.getCount();
case NEEDS_REVIEW -> needsReview += row.getCount();
default -> {
/* NOT_APPLICABLE already filtered */
}
}
if (lastDetectedAt == null || row.getLastDetectedAt().isAfter(lastDetectedAt)) {
lastDetectedAt = row.getLastDetectedAt();
}
}
}
}
Loading
Loading