feat(server): expose practice-group standings and feedback responses - #1486
Merged
Conversation
Contributor
|
Important Review skippedToo many files! This PR contains 404 files, which is 304 over the limit of 100. To get a review, reduce the PR to 100 files or fewer by splitting it into smaller PRs or changing its base branch. Upgrade to a paid plan to raise the limit. Usage-priced reviews support at most 300 files. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (404)
You can disable this status message by setting the 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 |
github-actions
Bot
added
documentation
Contributor
📚 Documentation Preview
|
This was referenced Aug 22, 2026
ValentinGruener
force-pushed
the
feat/practice-area-server
branch
4 times, most recently
from
August 24, 2026 05:29
382c636 to
87e2d3b
Compare
This was referenced Aug 24, 2026
ValentinGruener
force-pushed
the
feat/practice-area-server
branch
from
August 25, 2026 10:03
90c618a to
1c14fee
Compare
2 tasks
ValentinGruener
force-pushed
the
feat/practice-area-server
branch
2 times, most recently
from
August 27, 2026 11:23
ba92937 to
7135992
Compare
ValentinGruener
marked this pull request as ready for review
August 27, 2026 11:24
2 tasks
FelixTJDietrich
changed the title
FelixTJDietrich
changed the title
Adds the API a developer profile needs to explain a practice area: its current standing and guidance, the direction its recent evidence points, the feedback sources behind it, and the review moments that produced it. Trend direction is evidence-weighted and opportunity-indexed rather than time-boxed, so a practice recovers after two problem-free reviews instead of staying marked mixed for the rest of the window. Delivered feedback can be marked helpful or not helpful. Includes the regenerated OpenAPI spec and client, which must ship with the API change: the openapi-validation gate regenerates both and fails on drift.
Both levels now answer the same question — what share of the evidence was positive — and read the answer off the same scale, so an area can no longer contradict the cards it is built from. A practice standing is the recency-weighted share of positive outcomes across its four newest evidence opportunities. That replaces a pair of rules that disagreed about both unit and denominator: an existence test over items that could not tell one problem from fifty, plus a clean-streak override that could. The decay is derived rather than picked, so two problem-free work items in a row still acknowledge a fixed habit — and a fresh regression registers just as fast. An area averages the continuous practice standings and classifies once, instead of rounding each practice to a label and averaging those. A practice can now say why it has nothing to say: NOT_OBSERVED when no review reached it, NO_OPPORTUNITY when the reviews ran and found nothing to report. Carrying that on the practice lets the area be a pure roll-up and removes the separate evidence census that existed only to compensate. Also fixes the latest-run correlation on the learner surfaces: it now matches on practice and subject, not the artifact alone, so a review that never examined a practice no longer supersedes what an earlier one found there. The mentor aggregates keep the old grain deliberately and say so. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017XBuU2tfPFntHoE34Cmw1Z
An area summary treats every practice as equally important, which is rarely true: a team that cares most about test coverage cannot say so. Practices now carry an area weight that applies to both halves of the area view — the standing and the direction — so an area reads as the practices its workspace actually weighs. The trend already accepted per-practice weights and folded them into its inverse-variance precision; both call sites passed an empty map. They now pass the weights from the reflection snapshot, so the area status and the trend detail cannot disagree about how much a practice counts. The area standing moves from a plain average to the weighted one. Weight 0 withdraws a practice from its area's summary while leaving it fully reviewed and fully visible: not counting toward the area is a different statement from not being looked at, and the autonomy ladder already makes the latter. The default is neutral, so an unconfigured workspace is unaffected. Nothing about a practice's own card changes — a practice is never judged more or less harshly for being weighted, only listened to more or less when its area speaks for it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017XBuU2tfPFntHoE34Cmw1Z
The package was called reviewhistory but held both halves of the area detail
page: the review history AND the trend query service behind
/practice-areas/{areaSlug}/trend. Its own controller already told the truth —
PracticeAreaDetailController — so the package name was the odd one out, and a
reader opening reviewhistory had no reason to expect a trend service there.
Renamed to areadetail, with a package-info that records where the boundary
runs: the trend QUERY lives here because it answers for a screen, while the
trend MATHEMATICS stays in practices.observation.trend. Nothing outside the
package references any of these types, so the move is closed under the compiler
and no module boundary shifts.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017XBuU2tfPFntHoE34Cmw1Z
The rebase conflicted on openapi.yaml and the generated client, which are regenerated rather than merged. Resolving them to this branch's side dropped main's catalog-adoption endpoints and the deletePractices parameter; running both generators against the merged Java restores them alongside this branch's own additions. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017XBuU2tfPFntHoE34Cmw1Z
Three states were measured against the evidence this surface actually has and removed, and one boundary was moved to where the meaning changes. STABLE is gone. Claiming equivalence is a stronger claim than claiming difference: the posterior mass inside the practical-equivalence band has to clear the credibility threshold, which at four opportunities a bundle peaks around 0.70 and cannot. Reaching it needs roughly 12 opportunities a bundle for a practice at the extremes and 58 for one in the middle — against the four available. UNCERTAIN already carries the case honestly: no direction is supported, which is not the same as asserting there is none. SupportLevel is gone with it. Once both bundles must be full for a comparison to happen at all, it has one reachable value; and across all twenty combinations reachable at this bundle size, a three-item previous bundle never once changed a verdict against a four-item one, because the posterior already widens for the smaller sample. TrendSupport keeps its counts, which say the same thing without grading it. minBundleSize therefore rises to the bundle size, and the rule states in one line: the last four against the four before them. The standing's lower boundary moves from one half to 0.37, which is where the meaning changes rather than where the number is round. One problem on the newest work item, everything before it clean, scores 0.384; two in a row score 0.138. A boundary at one half put both on the same side, so for a developer working at an 80% success rate one review in five read as "needs attention" while the trend beside it stayed silent — correctly, since a single item is no evidence of change. Simulated over 20000 runs that fired 19.6% of the time; it now fires 9.6%, and at a 90% success rate 2.8% rather than 10.1%. The scale is symmetric again in the unit it reasons about: two clean opportunities earn a strength, two problems cost one, and a single event of either kind moves the standing without settling it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017XBuU2tfPFntHoE34Cmw1Z
…ody uses Rebasing onto main surfaced a break git could not: main removed the ArtifactKind import from ObservationService while this branch still reads the type at the artifact-kind filter. Both sides touched different lines of the same file, so the merge was clean and the compile was not — which is why it only appeared in CI, where the merge with main is what gets built. The twenty unused imports either side left behind now fail the build rather than warn, after main made the Java lint reject violations. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017XBuU2tfPFntHoE34Cmw1Z
Main now runs NullAway in JSpecify mode, so every package states its nullness and every dereference has to be justified. The rebase itself was almost free — two conflicts, both unions — and the work was in the annotations the analysis then demanded. Five packages this branch added or renamed were missing their @NullMarked package-info and defaulted to unannotated, which silently exempts them from the very check that was just turned on. PracticeTrend now says which of its fields are genuinely absent: both outcome vectors while the evidence is not comparable, and the posterior difference for INSUFFICIENT_EVIDENCE and for an area result, which aggregates rather than diffs. Where a flag and the value it guards were correlated only by convention, the correlation is now written down: a trend that produced a card is required to exist, partitioningBy is documented to always yield both keys, and the two filter-parameter records expose a pageable() that normalises once instead of leaving each controller to reconstruct it from components the compact constructor had already defaulted. Repository query parameters that the JPQL reads as ":x IS NULL OR ..." are marked nullable, because that is what the query says about them. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017XBuU2tfPFntHoE34Cmw1Z
The reflection query dropped every observation whose presence carried no verdict, so a second query had to count them back for the area status, with predicates that had to be kept identical by hand. It now takes a verdictsOnly flag: the mentor and agent context providers still ask for verdicts only, and the reflection surface asks for everything and filters one layer up, where the same pass that classifies every other row can also count these. That pass now groups on ObservationOutcome once instead of testing it two to four times per row, which makes the split visibly exhaustive: five outcomes, five buckets, nothing falling through. Feeding the verdictless rows onward revives the trend package's coverage handling, which could not fire while they never arrived. Two guards had to follow: the trail cap now counts opportunities that produced a verdict, so a quiet stretch cannot push real evidence out of the window a standing reads from, and the reported calendar span is dated from those same opportunities, so "these comparisons span N days" keeps meaning comparisons. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…nisms A developer had two ways to answer one piece of feedback: a reaction saying what they would do about it, and a rating saying whether it helped. The two disagreed about their own history — reactions accumulated, ratings were overwritten — so the same screen could show a dispute from Thursday next to a rating that had silently replaced Monday's. They are now one endpoint carrying two independent questions. Either may be answered alone, and a row records exactly what was said at that moment. That is what the research record needs: someone who rated a unit helpful on Monday and disputed it on Thursday said both things, and inventing a Monday dispute to make the newest row complete would be a worse record than none. The consequence is that the current answer is not the newest row. It is the newest value of each question independently, and every read had to learn that: the response endpoint, the engagement counts, the review-history card, and — the one that mattered — re-nag suppression, which would otherwise have started re-delivering a disputed finding the moment its recipient gave it a thumbs-up. That rule is written once as ReactionRepository.STILL_SPEAKS rather than four times, because the fourth copy is the one that gets forgotten. Append-only tables can only accumulate, so a mis-click would have been permanent. A row carrying neither answer is a withdrawal: it ends the run of rows before it and leaves nothing on record. It is a flag on the wire rather than an empty body, so a client that lost its state cannot erase an answer by asking for nothing. The reaction column keeps the name `action` — it shipped under that name, and a released column is renamed across two releases, not in passing. The field says what the value means. FeedbackResponseService reaches the caller through CurrentDeveloperLookup like its neighbours, which retires two frozen module-boundary violations rather than carrying them forward under a new class name. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017XBuU2tfPFntHoE34Cmw1Z
A working drawing, not a contributor doc. It joins the two research notes that are already local-only. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…absence Three findings from reviewing the branch, none of which local runs had shown. The full-context budget failed in CI and not here, because the local suite never runs SpringTestContextArchitectureTest — `pnpm run test:server:verification` is the command that does, and it is what CI runs. PracticeAreaStatusIntegrationTest earns a second application context: its nested class registers an AreaGuidanceProvider bean, and "a provider is present" cannot be tested from the same context as "none is registered". That is a reviewed cost, so it is now recorded as one with its justification rather than left to fail the gate. FeedbackResponseService read the caller through the throwing lookup on all three methods, including its two reads. CurrentDeveloperLookup says the opposite in as many words: absence is a normal answer for a read, and only a write has no meaningful empty result. Its two neighbours already keep that; this one now does too, so a signed-in person who was never synced as a developer sees no response and zero counts instead of an error. ObservationService still declared the visibility policy it stopped using when the reflection surface moved out. The field was injected and read nowhere. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017XBuU2tfPFntHoE34Cmw1Z
A changeset lands in CHANGELOG.md verbatim, so it may only describe what an operator can actually do. "practices can count for more in their area" says a practice can be weighted. areaWeight exists as a column and both halves of the area view read it, but nothing writes it — no endpoint, no DTO, no screen. Announcing a setting nobody can set would be the release note lying. "an area says which kind of silence it is" says the same thing as "a practice says why it has nothing to say", which already covers both levels and states that an area's standing is read straight off its practices. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017XBuU2tfPFntHoE34Cmw1Z
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>
PracticeAreaReviewArtifact.title is @nullable on the server and no longer required in openapi.yaml, but the generated client still declared it as always present. CI regenerates the client and fails on any diff, so the two had to be brought back in step. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
FelixTJDietrich
force-pushed
the
feat/practice-area-server
branch
from
August 27, 2026 16:31
7135992 to
b164e38
Compare
FelixTJDietrich
enabled auto-merge (squash)
August 27, 2026 22:14
FelixTJDietrich
added a commit
that referenced
this pull request
Aug 31, 2026
…ping MIGRATION.md The Version PR fails on every main push with 'MIGRATION.md already contains ### v0.75.0'. The latest published release is v0.74.0, but feature PRs merged before the fragment flow hand-wrote '### v0.75.0' (#1486) and '### v0.76.0' (#1614) headings for work that was never released, so the version changesets compute now collides with them. A version can only gain a history section by being released, and changeset version computes the lowest unreleased version — so any section at or above it, sitting directly below the '### Next release' anchor, is unreleased content that ships in the release being cut. Stamping now folds those sections into the new version section instead of failing, which also makes regeneration idempotent over the script's own prior output. The duplicate guard still rejects a same-version heading anywhere below released history, where it is a real conflict. Verified against main's actual MIGRATION.md: 'changeset version' plus this script produces one v0.75.0 section carrying all eleven entries (three pending, six mislabeled, two fragments) with no content loss, and a second run is byte-idempotent. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
2 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Adds the authenticated developer API for understanding how reviewed work reflects on each practice and practice group: current standings and guidance, recent direction, the observations behind those conclusions, and the review runs that produced them. Developers can also record, replace, or withdraw their current response to delivered practice feedback.
Direction compares opportunities to apply a practice rather than elapsed time. A practice can therefore recover after two problem-free reviews instead of remaining mixed because of older observations in the 90-day evidence window.
This PR intentionally ships the server contract before the new profile UI. #1487 will be rebased onto
mainand updated to consume this contract as an independent follow-up.REST resources
All developer-facing reads require authentication and return data only for the signed-in developer. Review runs retain undecided observations without turning them into positive or negative verdicts, and invalid pagination is rejected with
400rather than silently normalized.Feedback responses
A response can contain usefulness (
HELPFULorUNHELPFUL), resolution (ADDRESSED,DISPUTED, orNOT_APPLICABLE), or both. A dispute requires an explanation.PUTreplaces the current response,GETreads it, and idempotentDELETEwithdraws it. Storage remains append-only for auditability while the HTTP API exposes one current resource.Compatibility and migration
This release replaces practice-area and learner-projection terminology with practice-group and reviewed-practice contracts, and replaces the older reaction endpoints with the feedback-response resource. Direct API consumers must migrate as documented in
MIGRATION.md; the bundled generated client is updated in the same release.Liquibase applies the schema changes automatically. Existing feedback-response history and append-only configuration-audit history are preserved, and historical audit values are translated when read rather than rewritten. No manual data migration or new configuration is required.
server/openapi.yaml,webapp/src/api/**, and the ERD ship with their source changes because repository gates reject generated drift.How to test
Run the complete local repository gates:
Manual API smoke test as an authenticated workspace member:
GETno longer returns a resolution.GETreports no current response.400.Checklist
.changeset/README.md**Operators:** …) andMIGRATION.mdis updated