fix(server): compose feedback for review, profile, and chat

.changeset/a-review-stands-as-written.md

@@ -0,0 +1,5 @@
+---
+"hephaestus": patch
+---
+
+A review comment now stays as it was written. Where a later look at the same change used to rewrite the original comment in place — and quietly demote anything already answered on the diff — it now leaves a new comment beside the old one, the way a person would. The comment also stops repeating the notes that sit on the diff: those live on the lines they are about, and anything that could not be placed on a line falls back into the comment rather than disappearing between the two.

.changeset/mentor-notes-say-what-was-already-said.md

@@ -0,0 +1,5 @@
+---
+"hephaestus": patch
+---
+
+Notes prepared for the mentor now say where a point has already been put to the developer and whether anything has moved without help, so a conversation does not repeat feedback they have already had twice. Notes written before this carry no such record, which reads as nothing having been said rather than as nothing to say.

.changeset/review-comments-read-better.md

@@ -0,0 +1,5 @@
+---
+"hephaestus": patch
+---
+
+Review comments no longer repeat their own issue count in the opening line, no longer run a strength and its next step together into one unpunctuated sentence, and no longer report how long the run took.

.changeset/reviews-land-beside-the-code.md

@@ -0,0 +1,5 @@
+---
+"hephaestus": patch
+---
+
+A problem the review found by opening a file now arrives as a comment on the changed line, rather than as a paragraph at the bottom of the merge request.

.changeset/reviews-lead-with-what-matters.md

@@ -0,0 +1,5 @@
+---
+"hephaestus": patch
+---
+
+A review comment now leads with its most serious finding and the one edit that fixes it, instead of leading with whichever finding happened to have no line number attached. Each finding says what to do before it says why it matters, and the reasoning is one sentence rather than the same paragraph on every review that touches the practice.

.changeset/reviews-open-in-their-own-words.md

@@ -0,0 +1,5 @@
+---
+"hephaestus": patch
+---
+
+A review comment now opens with a sentence written about your change, instead of one of a handful of fixed lines that read the same on every review — including on reviews that opened with praise ahead of a serious problem. When the review has nothing worth opening on, it opens on its first finding.

.changeset/reviews-stop-quoting-the-catalogue.md

@@ -0,0 +1,5 @@
+---
+"hephaestus": patch
+---
+
+Review comments no longer append the workspace's own wording about a practice to each note. That paragraph was identical on every review that touched the practice, and it was about the practice rather than about the change in front of you. The practice still decides what gets raised; it just doesn't get quoted back.

server/src/main/java/de/tum/cit/aet/hephaestus/agent/context/providers/mentor/PreparedConversationFeedbackContentSource.java

@@ -146,6 +146,11 @@ private static void writeNotes(ObjectNode node, String body) {
         notes.put("capability", brief.capability());
         notes.put("evidenceSummary", brief.evidenceSummary());
         notes.put("inConversationSignal", brief.inConversationSignal());
+        // Absent on a brief written before the field existed, and absent when nothing has been put to them
+        // yet. Either way the mentor is told nothing rather than told there is nothing.
+        if (brief.alreadySaid() != null) {
+            notes.put("alreadySaid", brief.alreadySaid());
+        }
     }
 
     /**

server/src/main/java/de/tum/cit/aet/hephaestus/agent/handler/ApprovedFeedbackDeliveryListener.java

@@ -89,6 +89,24 @@ public void deliver(ApprovedFeedbackReadyEvent event) {
             }
             return;
         }
+        String approvedBody = feedback.getBody();
+        String safeBody = PullRequestCommentPoster.sanitize(approvedBody);
+        if (safeBody.isBlank()) {
+            feedbackRepository.markApprovedSuppressed(
+                event.workspaceId(),
+                feedback.getId(),
+                FeedbackSuppressionReason.EMPTY_AFTER_SANITIZE.name()
+            );
+            return;
+        }
+        if (!safeBody.equals(approvedBody)) {
+            feedbackRepository.markApprovedSuppressed(
+                event.workspaceId(),
+                feedback.getId(),
+                FeedbackSuppressionReason.APPROVAL_STALE.name()
+            );
+            return;
+        }
         ExistingDeliveryLookup existing = commentPoster.findApprovedProposal(job, feedback.getId());
         if (existing.kind() == ExistingDeliveryLookup.Kind.UNKNOWN) {
             log.warn("Approved proposal deferred after inconclusive provider lookup: feedbackId={}", feedback.getId());

server/src/main/java/de/tum/cit/aet/hephaestus/agent/handler/FeedbackDeliveryService.java

@@ -12,11 +12,8 @@
 import de.tum.cit.aet.hephaestus.practices.observation.ObservationTrendService;
 import de.tum.cit.aet.hephaestus.practices.observation.TrendDelta;
 import de.tum.cit.aet.hephaestus.practices.review.PracticeReviewProperties;
-import java.util.ArrayList;
 import java.util.List;
 import java.util.Objects;
-import java.util.Set;
-import java.util.stream.Collectors;
 import org.jspecify.annotations.Nullable;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
@@ -52,10 +49,6 @@ class FeedbackDeliveryService {
         this.commentFormatter = commentFormatter;
     }
 
-    void deliverFeedback(AgentJob job, @Nullable DeliveryContent delivery) {
-        deliverFeedback(job, delivery, null);
-    }
-
     void recordProposal(
         AgentJob job,
         @Nullable DeliveryContent delivery,
@@ -68,17 +61,7 @@ ExistingDeliveryLookup findExistingDeliveryCommentId(AgentJob job) {
         return commentPoster.findExistingSummaryComment(job);
     }
 
-    @FunctionalInterface
-    interface InlineAwareSummaryComposer {
-        @Nullable
-        String compose(Set<String> deliveredObservationFingerprints);
-    }
-
-    void deliverFeedback(
-        AgentJob job,
-        @Nullable DeliveryContent delivery,
-        @Nullable InlineAwareSummaryComposer summaryComposer
-    ) {
+    void deliverFeedback(AgentJob job, @Nullable DeliveryContent delivery) {
         if (delivery == null) {
             log.debug("No delivery content, skipping: jobId={}", job.getId());
             return;
@@ -96,7 +79,7 @@ void deliverFeedback(
         }
 
         try {
-            doDeliverEligible(job, delivery, summaryComposer, Objects.requireNonNull(decision.artifact()));
+            doDeliverEligible(job, delivery, Objects.requireNonNull(decision.artifact()));
         } catch (JobDeliverySuppressedException e) {
             log.info("Delivery suppressed at egress: jobId={}", job.getId());
             recordGateSuppressed(job, delivery, FeedbackSuppressionReason.INSTANCE_SILENCED);
@@ -125,12 +108,7 @@ private void recordPartialSummaryDelivery(AgentJob job, DeliveryContent delivery
         }
     }
 
-    private void doDeliverEligible(
-        AgentJob job,
-        DeliveryContent delivery,
-        @Nullable InlineAwareSummaryComposer summaryComposer,
-        PullRequest pullRequest
-    ) {
+    private void doDeliverEligible(AgentJob job, DeliveryContent delivery, PullRequest pullRequest) {
         TrendDelta trend = reviewProperties.progressFooter()
             ? observationTrendService
                   .computeForTarget(ArtifactKinds.PULL_REQUEST, pullRequest.getId(), job.getWorkspace().getId())
@@ -153,9 +131,6 @@ private void doDeliverEligible(
         }
         List<InlineFeedbackChannel.DeliveredSignal> inlineSignals = inlineResult.signals();
 
-        if (summaryOutcome == SummaryOutcome.DELIVERED && !inlineResult.suppressed()) {
-            reEditSummaryWithSignals(job, summaryComposer, inlineSignals, trend);
-        }
         boolean inlineDelivered = inlineResult.posted() > 0;
         if (inlineResult.suppressed() && summaryOutcome != SummaryOutcome.DELIVERED && !inlineDelivered) {
             recordGateSuppressed(job, delivery, FeedbackSuppressionReason.INSTANCE_SILENCED);
@@ -241,7 +216,6 @@ private void recordGateSuppressed(
     private enum SummaryOutcome {
         DELIVERED,
         NOT_REQUIRED,
-        TRANSIENT_NOOP,
         SKIPPED_EMPTY,
     }
 
@@ -258,124 +232,21 @@ private SummaryOutcome postSummaryNote(AgentJob job, DeliveryContent delivery, @
         String footer = ProgressFooterRenderer.render(trend);
         String body = footer.isEmpty() ? sanitized : sanitized + "\n\n" + footer;
         String formatted = commentFormatter.format(body, job);
-        String priorRef = feedbackLedgerRecorder.priorLiveSummaryRef(job).orElse(null);
-        PullRequestCommentPoster.UpdateResult update =
-            priorRef == null ? null : commentPoster.updateFormattedBody(job, priorRef, formatted);
-
-        if (update != null && update.kind() == PullRequestCommentPoster.UpdateResult.Kind.TRANSIENT) {
-            // A transient edit failure must not create a duplicate summary.
-            job.setDeliveryCommentId(Objects.requireNonNull(priorRef));
-            log.warn(
-                "Summary edit transient — kept prior summary, no fresh post: jobId={}, commentId={}",
-                job.getId(),
-                priorRef
-            );
-            return SummaryOutcome.TRANSIENT_NOOP;
-        }
 
-        boolean editedInPlace = update != null && update.kind() == PullRequestCommentPoster.UpdateResult.Kind.EDITED;
-        String commentId = editedInPlace
-            ? Objects.requireNonNull(update).externalId()
-            : commentPoster.postFormattedBody(job, formatted);
+        // A review that has been posted stays as it was written. A later look at the same change leaves a
+        // new comment beside it, the way a person would, and the composer is told what it already said so
+        // the new one reads as a second visit rather than a repeat.
+        String commentId = commentPoster.postFormattedBody(job, formatted);
         if (commentId == null) {
             throw new JobDeliveryException(
                 "Summary note post returned no comment id despite a non-empty body: jobId=" + job.getId()
             );
         }
         job.setDeliveryCommentId(commentId);
-        log.info(
-            "Practice summary note delivered: jobId={}, commentId={}, editedInPlace={}",
-            job.getId(),
-            commentId,
-            editedInPlace
-        );
-        if (editedInPlace && trend != null && trend.hasMeaningfulChange()) {
-            postReReviewPing(job, trend);
-        }
+        log.info("Practice summary note delivered: jobId={}, commentId={}", job.getId(), commentId);
         return SummaryOutcome.DELIVERED;
     }
 
-    private void reEditSummaryWithSignals(
-        AgentJob job,
-        @Nullable InlineAwareSummaryComposer summaryComposer,
-        List<InlineFeedbackChannel.DeliveredSignal> inlineSignals,
-        @Nullable TrendDelta trend
-    ) {
-        String summaryRef = job.getDeliveryCommentId();
-        if (summaryComposer == null || summaryRef == null) {
-            return;
-        }
-        Set<String> deliveredKeys = inlineSignals
-            .stream()
-            .filter(signal -> signal.disposition() != InlineFeedbackChannel.Disposition.FAILED)
-            .map(InlineFeedbackChannel.DeliveredSignal::recurrenceKey)
-            .filter(key -> key != null && !key.isBlank())
-            .collect(Collectors.toSet());
-        if (deliveredKeys.isEmpty()) {
-            return;
-        }
-
-        String demoted = summaryComposer.compose(deliveredKeys);
-        if (demoted == null) {
-            return;
-        }
-        String sanitized = PullRequestCommentPoster.sanitize(demoted);
-        if (sanitized.isBlank()) {
-            return;
-        }
-        String footer = ProgressFooterRenderer.render(trend);
-        String body = footer.isEmpty() ? sanitized : sanitized + "\n\n" + footer;
-        String formatted = commentFormatter.format(body, job);
-
-        try {
-            PullRequestCommentPoster.UpdateResult update = commentPoster.updateFormattedBody(
-                job,
-                summaryRef,
-                formatted
-            );
-            if (update.kind() == PullRequestCommentPoster.UpdateResult.Kind.EDITED) {
-                log.info(
-                    "Summary demoted in place after inline delivery: jobId={}, commentId={}",
-                    job.getId(),
-                    summaryRef
-                );
-            } else {
-                log.debug(
-                    "Summary demotion did not land ({}); keeping full-line summary: jobId={}",
-                    update.kind(),
-                    job.getId()
-                );
-            }
-        } catch (RuntimeException e) {
-            log.warn("Summary demotion failed (delivery unaffected): jobId={}, error={}", job.getId(), e.getMessage());
-        }
-    }
-
-    private void postReReviewPing(AgentJob job, TrendDelta trend) {
-        List<String> parts = new ArrayList<>();
-        if (trend.countResolved() > 0) {
-            parts.add(trend.countResolved() + " resolved");
-        }
-        if (trend.countNew() > 0) {
-            parts.add(trend.countNew() + " new");
-        }
-        if (trend.countRegressed() > 0) {
-            parts.add(trend.countRegressed() + " slipped back");
-        }
-        String body =
-            "<!-- hephaestus:re-review-ping:" +
-            job.getId() +
-            " -->\n🔁 **Re-reviewed** — " +
-            String.join(", ", parts) +
-            ". See the updated review summary above.";
-        try {
-            String pingId = commentPoster.postFormattedBody(job, body);
-            log.info("Re-review ping posted: jobId={}, pingCommentId={}", job.getId(), pingId);
-        } catch (RuntimeException e) {
-            log.warn("Re-review ping failed (delivery unaffected): jobId={}, error={}", job.getId(), e.getMessage());
-        }
-    }
-
     private DiffNotePoster.DiffNoteResult postDiffNotes(AgentJob job, DeliveryContent delivery) {
         // Empty reconciliation must still remove stale inline notes after policy guards pass.
         DiffNotePoster.DiffNoteResult diffResult = diffNotePoster.reconcileInlineNotes(job, delivery.diffNotes());
@@ -389,6 +260,10 @@ private DiffNotePoster.DiffNoteResult postDiffNotes(AgentJob job, DeliveryConten
         return diffResult;
     }
 
+    ExistingDeliveryLookup findExistingSummary(AgentJob job) {
+        return commentPoster.findExistingSummaryComment(job);
+    }
+
     private void recordUndelivered(AgentJob job, DeliveryContent delivery) {
         try {
             feedbackLedgerRecorder.recordUndelivered(job, delivery);

server/src/main/java/de/tum/cit/aet/hephaestus/agent/handler/FeedbackLedgerRecorder.java

@@ -578,7 +578,9 @@ public void recordWithheld(AgentJob job, Observation observation, FeedbackSuppre
     @Transactional(propagation = Propagation.REQUIRES_NEW)
     public void recordProposal(AgentJob job, @Nullable DeliveryContent delivery, List<ValidatedObservation> proposed) {
         final int position = 7_000;
-        if (delivery == null || delivery.mrNote() == null || delivery.mrNote().isBlank()) return;
+        if (delivery == null || delivery.mrNote() == null) return;
+        String body = PullRequestCommentPoster.sanitize(delivery.mrNote());
+        if (body.isBlank()) return;
         if (feedbackRepository.existsByAgentJobIdAndPosition(job.getId(), position)) return;
         Map<String, Observation> stored = observationRepository
             .findByAgentJobId(job.getId())
@@ -610,7 +612,7 @@ public void recordProposal(AgentJob job, @Nullable DeliveryContent delivery, Lis
                 .channel(FeedbackChannel.IN_CONTEXT)
                 .position(position)
                 .deliveryState(FeedbackDeliveryState.AWAITING_APPROVAL)
-                .body(delivery.mrNote())
+                .body(body)
                 .source(FeedbackSource.AGENT)
                 .createdAt(Instant.now())
                 .build()

server/src/main/java/de/tum/cit/aet/hephaestus/agent/handler/IssueReviewHandler.java

@@ -264,12 +264,17 @@ public void deliver(AgentJob job) {
         );
         Map<String, String> why = practiceCatalogInjector.whyBySlug(job.getWorkspace(), ArtifactKinds.ISSUE);
         List<ComposedFeedbackUnit> units = compositionResultParser.parse(job.getOutput(), FeedbackChannel.IN_CONTEXT);
+        String lead = compositionResultParser.lead(job.getOutput());
+        var note = DeliveryComposer.compose(loudEnough, ArtifactKinds.ISSUE, why, null, units, lead);
+        // An approved proposal posts as its own comment, so only one of the two may open on the lead.
+        // The auto-posted note is composed first and takes it; the proposal takes it only when there is
+        // no such note, which is the all-approval workspace where the proposal is the only comment.
         feedbackLedgerRecorder.recordProposal(
             job,
-            DeliveryComposer.compose(proposals, ArtifactKinds.ISSUE, why, null, units),
+            DeliveryComposer.compose(proposals, ArtifactKinds.ISSUE, why, null, units, note == null ? lead : null),
             proposals
         );
-        postIssueNote(job, DeliveryComposer.compose(loudEnough, ArtifactKinds.ISSUE, why, null, units));
+        postIssueNote(job, note);
     }
 
     private PracticeDetectionResultParser.ValidatedObservation validated(Observation observation) {

server/src/main/java/de/tum/cit/aet/hephaestus/agent/handler/PracticeFeedbackCommentFormatter.java

@@ -2,7 +2,6 @@
 
 import de.tum.cit.aet.hephaestus.agent.job.AgentJob;
 import de.tum.cit.aet.hephaestus.config.ApplicationProperties;
-import java.time.Duration;
 import org.jspecify.annotations.Nullable;
 import org.springframework.stereotype.Component;
 import org.springframework.web.util.HtmlUtils;
@@ -56,12 +55,7 @@ private static void appendMetadataFooter(StringBuilder sb, AgentJob job) {
         if (modelName != null && !modelName.isBlank()) {
             sb.append(" · ").append(HtmlUtils.htmlEscape(modelName));
         }
-        if (job.getStartedAt() != null && job.getCompletedAt() != null) {
-            sb.append(" · ").append(formatDuration(Duration.between(job.getStartedAt(), job.getCompletedAt())));
-        }
-
-        sb.append("</sub>\n");
-        sb.append("<sub>AI-generated feedback can be inaccurate. React with 👍 or 👎 to give feedback.</sub>\n");
+        sb.append(" &middot; AI-generated and can be inaccurate. React with 👍 or 👎 to give feedback.</sub>\n");
     }
 
     @Nullable
@@ -72,14 +66,4 @@ private static String snapshotModelName(@Nullable JsonNode configSnapshot) {
         JsonNode model = configSnapshot.path("upstreamModelId");
         return model.isString() ? model.asString() : null;
     }
-
-    private static String formatDuration(Duration duration) {
-        long totalSeconds = Math.max(0, duration.toSeconds());
-        if (totalSeconds < 60) {
-            return totalSeconds + "s";
-        }
-        long minutes = totalSeconds / 60;
-        long seconds = totalSeconds % 60;
-        return minutes + "m " + seconds + "s";
-    }
 }