fix(server): keep a comment that came without a resolution

The explanation subquery looked for the newest row carrying a resolution,
so a comment written alongside usefulness alone was stored and then never
read back — and a later resolution overwrote the reading with its own empty
comment. It now looks for the newest row carrying an explanation, which is
the column it was always after. Both readers had the same filter.

Area trends pooled their practices with 1/Sum(w/v), which is the variance of
an inverse-variance mean only when every weight is 1. A weight of 2 halved
the variance it reported and turned an uncertain area into a confident
verdict on the strength of an admin setting. The numerator now carries
Sum(w^2/v) so a uniform rescaling leaves the direction alone.

GraphQlOperationDocumentValidationTest scoped its GitHub carve-out by
matching the English wording of the validation message. graphql-java
localises those, so the build went red on a German workstation while CI
stayed green. It now reads the two field paths, which are part of the query
rather than of the translation.

Also folds the shared query-filter parsing into practices/web and renames
the branch changelog so its timestamp sorts after the released ones.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Author: 2 people

.changeset/an-answer-keeps-the-words-that-came-with-it.md

@@ -0,0 +1,7 @@
+---
+"hephaestus": patch
+---
+
+A comment written alongside "not helpful" is no longer lost. Explaining why a piece of feedback missed the mark, without also saying whether you addressed or disputed it, stored the text and then never showed it again — the next answer overwrote the reading with its own empty comment. The words now stay with the answer they came with.
+
+Areas whose practices carry different weights also stop overstating how sure they are: a weight above one made an area's trend read as a confident improvement or decline where the evidence only supported "uncertain".

server/openapi.yaml

@@ -11382,7 +11382,6 @@ components:
           type: string
       required:
       - id
-      - title
       - type
     PracticeAreaReviewFinding:
       type: object

server/src/main/java/de/tum/cit/aet/hephaestus/practices/areadetail/PracticeAreaReviewHistoryFilterParams.java

@@ -2,31 +2,21 @@
 
 import de.tum.cit.aet.hephaestus.integration.core.signal.ArtifactKind;
 import de.tum.cit.aet.hephaestus.practices.model.Severity;
+import de.tum.cit.aet.hephaestus.practices.web.QueryFilterSupport;
 import io.swagger.v3.oas.annotations.Parameter;
 import java.util.List;
-import java.util.Objects;
 import org.jspecify.annotations.Nullable;
-import org.springframework.data.domain.PageRequest;
 import org.springframework.data.domain.Pageable;
-import org.springframework.http.HttpStatus;
 import org.springframework.web.bind.annotation.RequestParam;
-import org.springframework.web.server.ResponseStatusException;
 
 /**
  * Query parameters for the learner-facing practice-area review history.
  *
- * <p>Optional components are nullable wrappers defaulted in the compact constructor: a
- * {@code defaultValue} on {@code @RequestParam} does not reach a record bound as a
- * {@code @ParameterObject}, so a primitive component would receive {@code null} and answer 400 to a
- * request that named no filter at all.
+ * <p>Every optional component is a nullable wrapper, and {@link QueryFilterSupport} states why, along
+ * with how a raw artifact kind and a raw page become the values this surface reads.
  */
 public record PracticeAreaReviewHistoryFilterParams(
     @RequestParam(required = false) @Nullable String practiceSlug,
-    /**
-     * Bare strings, not {@link ArtifactKind}s: springdoc publishes a typed parameter as
-     * {@code artifactKinds.value}, so a generated client would send a query key no caller writes. The
-     * grammar is enforced in {@link #kinds()}, where a malformed value becomes a 400.
-     */
     @Parameter(description = "Only reviews of these artifact kinds, e.g. scm.pull_request (repeatable)")
     @RequestParam(required = false)
     @Nullable
@@ -38,31 +28,16 @@ public record PracticeAreaReviewHistoryFilterParams(
     Integer page,
     @Parameter(description = "Page size, clamped to 1..50") @RequestParam(required = false) @Nullable Integer size
 ) {
-    public PracticeAreaReviewHistoryFilterParams {
-        page = page == null || page < 0 ? 0 : page;
-        size = size == null ? 10 : Math.clamp(size, 1, 50);
-    }
+    private static final int DEFAULT_PAGE_SIZE = 10;
+    private static final int MAX_PAGE_SIZE = 50;
 
-    /**
-     * The page to read, already normalised.
-     *
-     * <p>The compact constructor above defaults a missing or negative page to the first one and clamps the
-     * size, so neither component is null by the time anything reads it. Returning the {@link Pageable} here
-     * rather than the two numbers keeps that normalisation in one place and out of the controller.
-     */
+    /** The page to read, already normalised. */
     public Pageable pageable() {
-        return PageRequest.of(Objects.requireNonNull(page), Objects.requireNonNull(size));
+        return QueryFilterSupport.pageable(page, size, DEFAULT_PAGE_SIZE, MAX_PAGE_SIZE);
     }
 
     /** The requested artifact kinds, parsed; {@code null} means "every kind". */
     public @Nullable List<ArtifactKind> kinds() {
-        if (artifactKinds == null) {
-            return null;
-        }
-        try {
-            return artifactKinds.stream().map(ArtifactKind::of).toList();
-        } catch (IllegalArgumentException invalid) {
-            throw new ResponseStatusException(HttpStatus.BAD_REQUEST, invalid.getMessage(), invalid);
-        }
+        return QueryFilterSupport.artifactKinds(artifactKinds);
     }
 }

server/src/main/java/de/tum/cit/aet/hephaestus/practices/areadetail/PracticeAreaReviewHistoryService.java

@@ -87,7 +87,9 @@ public Page<PracticeAreaReviewMomentDTO> list(
             pageable
         );
         if (runs.isEmpty()) {
-            return Page.empty(pageable);
+            // Empty content, but the count the database made: a page read past the last one still has to say
+            // how many runs there are, or a stale deep link reads as "this area was never reviewed".
+            return new PageImpl<>(List.of(), pageable, runs.getTotalElements());
         }
 
         List<UUID> jobIds = runs.stream().map(ReviewHistoryRunRow::getJobId).toList();
@@ -170,7 +172,7 @@ private PracticeAreaReviewMomentDTO toMoment(
         PracticeAreaReviewArtifactDTO artifact =
             target == null
                 ? PracticeAreaReviewArtifactDTO.fallback(first.getArtifactKind(), first.getArtifactId())
-                : PracticeAreaReviewArtifactDTO.from(target, first.getArtifactKind(), first.getArtifactId());
+                : PracticeAreaReviewArtifactDTO.from(target, first.getArtifactId());
         List<PracticeAreaReviewFindingDTO> findings = observations
             .stream()
             .map(observation -> {

server/src/main/java/de/tum/cit/aet/hephaestus/practices/areadetail/dto/PracticeAreaReviewArtifactDTO.java

@@ -2,7 +2,6 @@
 
 import de.tum.cit.aet.hephaestus.integration.core.signal.ArtifactKind;
 import de.tum.cit.aet.hephaestus.integration.core.spi.IntegrationKind;
-import de.tum.cit.aet.hephaestus.practices.model.ArtifactKinds;
 import de.tum.cit.aet.hephaestus.practices.spi.ReviewRunTargetLookup.Target;
 import io.swagger.v3.oas.annotations.media.Schema;
 import org.jspecify.annotations.NonNull;
@@ -14,12 +13,12 @@ public record PracticeAreaReviewArtifactDTO(
     @NonNull Long id,
     @Nullable IntegrationKind provider,
     @Nullable Integer number,
-    @NonNull String title,
+    @Nullable String title,
     @Nullable String repositoryName,
     @Nullable String channelName,
     @Nullable String url
 ) {
-    public static PracticeAreaReviewArtifactDTO from(Target target, ArtifactKind fallbackType, Long fallbackId) {
+    public static PracticeAreaReviewArtifactDTO from(Target target, Long fallbackId) {
         return new PracticeAreaReviewArtifactDTO(
             target.type(),
             target.id() == null ? fallbackId : target.id(),
@@ -32,25 +31,12 @@ public static PracticeAreaReviewArtifactDTO from(Target target, ArtifactKind fal
         );
     }
 
-    public static PracticeAreaReviewArtifactDTO fallback(ArtifactKind type, Long id) {
-        return new PracticeAreaReviewArtifactDTO(type, id, null, null, fallbackTitle(type), null, null, null);
-    }
-
     /**
-     * ArtifactKind is an open {@code <domain>.<kind>} vocabulary rather than an enum, so this maps the kinds
-     * that reach the review history and falls back to the raw kind for anything a later module introduces —
-     * an unnamed kind must still render, not crash the page.
+     * The artifact of a run whose job row is gone. Only its identity survives, so every descriptive field is
+     * absent rather than invented: a null title says the title is unknown, where "Pull request" would claim
+     * that is what the work is called.
      */
-    private static String fallbackTitle(ArtifactKind type) {
-        if (ArtifactKinds.PULL_REQUEST.equals(type)) {
-            return "Pull request";
-        }
-        if (ArtifactKinds.ISSUE.equals(type)) {
-            return "Issue";
-        }
-        if (ArtifactKinds.CONVERSATION_THREAD.equals(type)) {
-            return "Conversation";
-        }
-        return type.value();
+    public static PracticeAreaReviewArtifactDTO fallback(ArtifactKind type, Long id) {
+        return new PracticeAreaReviewArtifactDTO(type, id, null, null, null, null, null, null);
     }
 }

server/src/main/java/de/tum/cit/aet/hephaestus/practices/feedback/FeedbackObservationRepository.java

@@ -162,7 +162,7 @@ LEFT JOIN LATERAL (
                               ORDER BY r.created_at DESC, r.id DESC LIMIT 1) AS action,
                             (SELECT r.explanation FROM reaction r
                               WHERE r.feedback_id = f.id AND r.reactor_user_id = :recipientUserId
-                                AND r.action IS NOT NULL
+                                AND r.explanation IS NOT NULL
             """ +
             ReactionRepository.STILL_SPEAKS +
             """

server/src/main/java/de/tum/cit/aet/hephaestus/practices/observation/ObservationFeedFilterParams.java

@@ -3,25 +3,23 @@
 import de.tum.cit.aet.hephaestus.integration.core.signal.ArtifactKind;
 import de.tum.cit.aet.hephaestus.practices.model.Presence;
 import de.tum.cit.aet.hephaestus.practices.model.Severity;
+import de.tum.cit.aet.hephaestus.practices.web.QueryFilterSupport;
 import io.swagger.v3.oas.annotations.Parameter;
 import java.util.List;
 import java.util.Objects;
 import org.jspecify.annotations.Nullable;
 import org.springframework.data.domain.PageRequest;
 import org.springframework.data.domain.Pageable;
 import org.springframework.data.domain.Sort;
-import org.springframework.http.HttpStatus;
 import org.springframework.web.bind.annotation.RequestParam;
-import org.springframework.web.server.ResponseStatusException;
 
 /**
  * Query parameters for the developer observation feed. Bound with {@code @ParameterObject}, so the wire
  * format stays one flat query string — this only changes the Java signature, not the HTTP contract.
  *
- * <p>Every optional component is a nullable wrapper, and the compact constructor supplies the defaults.
- * A {@code defaultValue} on {@code @RequestParam} does not reach a record bound this way: the binder
- * constructs the record and hands a primitive component {@code null}, which fails conversion and answers
- * 400 to a request that named no filter at all.
+ * <p>Every optional component is a nullable wrapper; the compact constructor supplies the ordering
+ * defaults and {@link QueryFilterSupport} the paging ones, and states why neither can come from a
+ * {@code defaultValue} on {@code @RequestParam}.
  */
 public record ObservationFeedFilterParams(
     @Parameter(description = "Filter by practice slug") @RequestParam(required = false) @Nullable String practiceSlug,
@@ -31,9 +29,8 @@ public record ObservationFeedFilterParams(
     String areaSlug,
     @Parameter(description = "Filter by presence") @RequestParam(required = false) @Nullable Presence presence,
     /**
-     * Bare strings, not {@link ArtifactKind}s: springdoc walks into the record and publishes a typed
-     * parameter as {@code artifactKinds.value}, so a generated client would send a query key no caller
-     * writes. The grammar is enforced in {@link #toQuery()} instead, where a malformed value becomes a 400.
+     * Bare strings, not {@link ArtifactKind}s — {@link QueryFilterSupport#artifactKind} has the reason,
+     * and parses them in {@link #toQuery()}, where a malformed value becomes a 400.
      */
     @Parameter(description = "Only observations on these artifact kinds, e.g. scm.pull_request (repeatable)")
     @RequestParam(required = false)
@@ -59,25 +56,23 @@ public record ObservationFeedFilterParams(
     Integer page,
     @Parameter(description = "Page size, clamped to 1..100") @RequestParam(required = false) @Nullable Integer size
 ) {
+    private static final int DEFAULT_PAGE_SIZE = 20;
+    private static final int MAX_PAGE_SIZE = 100;
+
     public ObservationFeedFilterParams {
         displayableOnly = displayableOnly != null && displayableOnly;
         sort = sort == null ? ObservationService.ObservationSort.DATE : sort;
         direction = direction == null ? Sort.Direction.DESC : direction;
-        // Clamped, not rejected: a feed is a reading surface, and answering 400 to "?size=999" tells a
-        // reader nothing they can act on while hiding data they are allowed to see.
-        page = page == null || page < 0 ? 0 : page;
-        size = size == null ? 20 : Math.clamp(size, 1, 100);
     }
 
     /**
      * The page to read, already normalised and sorted.
      *
-     * <p>The compact constructor above defaults every paging component, so none is null by the time
-     * anything reads it. The severity query carries its own ORDER BY, so it must not also receive a sort:
-     * adding one would have the database order by two different keys.
+     * <p>The severity query carries its own ORDER BY, so it must not also receive a sort: adding one
+     * would have the database order by two different keys.
      */
     public Pageable pageable() {
-        PageRequest page = PageRequest.of(Objects.requireNonNull(this.page), Objects.requireNonNull(size));
+        Pageable page = QueryFilterSupport.pageable(this.page, size, DEFAULT_PAGE_SIZE, MAX_PAGE_SIZE);
         return sort() == ObservationService.ObservationSort.SEVERITY
             ? page
             : PageRequest.of(
@@ -93,22 +88,11 @@ public ObservationFeedQuery toQuery() {
             practiceSlug,
             areaSlug,
             presence,
-            parseArtifactKinds(),
+            QueryFilterSupport.artifactKinds(artifactKinds),
             severities,
             Objects.requireNonNull(displayableOnly),
             Objects.requireNonNull(sort),
             direction == Sort.Direction.DESC
         );
     }
-
-    private @Nullable List<ArtifactKind> parseArtifactKinds() {
-        if (artifactKinds == null) {
-            return null;
-        }
-        try {
-            return artifactKinds.stream().map(ArtifactKind::of).toList();
-        } catch (IllegalArgumentException invalid) {
-            throw new ResponseStatusException(HttpStatus.BAD_REQUEST, invalid.getMessage(), invalid);
-        }
-    }
 }

server/src/main/java/de/tum/cit/aet/hephaestus/practices/observation/ObservationRepository.java

@@ -483,6 +483,14 @@ Page<ReviewHistoryRunRow> findReviewHistoryRuns(
         Pageable pageable
     );
 
+    /**
+     * The findings of the runs {@link #findReviewHistoryRuns} returned.
+     *
+     * <p>Fetches both revisions because every row is handed straight to {@code ObservationVisibilityPolicy},
+     * which reads the evaluated revision and the practice's current one to decide whether the claim still
+     * speaks for the practice — lazily, that is one round trip per practice and revision on the page.
+     */
+    @EntityGraph(attributePaths = { "practice.currentRevision", "practiceRevision" })
     @Query(
         """
         SELECT o FROM Observation o

server/src/main/java/de/tum/cit/aet/hephaestus/practices/observation/reaction/ReactionRepository.java

@@ -74,7 +74,7 @@ AND NOT EXISTS (
                   ORDER BY r.created_at DESC, r.id DESC LIMIT 1) AS "resolution",
                 (SELECT r.explanation FROM reaction r
                   WHERE r.feedback_id = :feedbackId AND r.reactor_user_id = :reactorUserId
-                    AND r.action IS NOT NULL
+                    AND r.explanation IS NOT NULL
             """ +
             STILL_SPEAKS +
             """

server/src/main/java/de/tum/cit/aet/hephaestus/practices/observation/trend/AreaTrendAggregator.java

@@ -100,15 +100,22 @@ private record Pooled(double mean, double variance) {}
     private static Pooled pool(List<PracticeTrend> comparable, Map<String, Double> weights) {
         double weightedMean = 0.0;
         double totalPrecision = 0.0;
+        double varianceNumerator = 0.0;
         for (PracticeTrend trend : comparable) {
             // `comparable` was filtered on difference() != null above; the nullness analysis
             // does not carry that across the stream boundary.
             BetaPosterior.Difference difference = Objects.requireNonNull(trend.difference());
-            double precision = weightFor(trend.slug(), weights) / difference.variance();
+            double weight = weightFor(trend.slug(), weights);
+            double precision = weight / difference.variance();
             weightedMean += precision * difference.mean();
             totalPrecision += precision;
+            // Var(Σ pᵢmᵢ / Σ pᵢ) = Σ(pᵢ²·vᵢ) / (Σ pᵢ)², and pᵢ²·vᵢ collapses to wᵢ²/vᵢ = wᵢ·pᵢ.
+            // Only when every weight is 1 does that reduce to the familiar 1/Σ pᵢ, so the
+            // numerator has to be carried separately — a weight of 2 would otherwise report
+            // half the variance it has and turn an UNCERTAIN area into a confident verdict.
+            varianceNumerator += weight * precision;
         }
-        return new Pooled(weightedMean / totalPrecision, 1.0 / totalPrecision);
+        return new Pooled(weightedMean / totalPrecision, varianceNumerator / (totalPrecision * totalPrecision));
     }
 
     /**