feat(AF-653): expose approval prediction on the read side - #705
Merged
Conversation
Adds the advisory approval-outcome prediction (AF-645) to the three read
surfaces and nothing else, which is what keeps the advisory-only guarantee
structural:
- GET /queries/{id} gains an approval_prediction block, joined in
DefaultQueryRequestLookupService. There is no reciprocal FK on
query_requests, so this is one unconditional unique-index lookup per
detail fetch.
- GET /reviews/pending gains a nullable approval_probability per row,
resolved for the whole page in a single findByQueryRequestIds call.
- RealtimeEventDispatcher consumes ApprovalPredictionCompletedEvent and
pushes query.prediction_complete to the eligible reviewers plus the
submitter.
QueryDetailView and ReviewService.PendingReview grow a component each;
QueryDetailView keeps a backward-compatible 25-arg constructor so the
existing call sites are untouched.
Closes #653
…-side Review follow-ups: - docs said the prediction push targets the query's lowest *open* stage; eligibleReviewersForLowestStage actually takes the review plan's minimum stage unconditionally. Corrected both docs and recorded why resolving the open stage is not worth it: it needs the decision list and the routing engine's effective min_approvals, both behind workflow-internal beans the realtime module cannot reach, for what is only a refetch hint. - Added the event to the WS registry table in docs/05-backend.md. - Registered query.prediction_complete in the frontend WS contract (WsEventName, WsEventPayloadMap, WS_EVENT_NAMES). Without it websocketManager hard-drops every frame with a console warning, so the event would be inert until #654. The badge and panel remain #654's scope. - Dropped the unreachable Collectors.toMap merge function; query_request_id is UNIQUE, so a duplicate key should throw rather than resolve silently.
Contributor
Frontend Test Results1 333 tests 1 333 ✅ 4m 48s ⏱️ Results for commit 63aa476. |
Contributor
Coverage Report for Frontend Coverage (frontend)
File CoverageNo changed files found. |
Contributor
Contributor
Backend Code Coverage
|
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
1 participant
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.
Closes #653. Part of epic #645 (approval-outcome prediction).
Everything below the read side already merged (#646–#652): the
approval_predictionstable, thecore.apiservices, the serving path and the retrain job. Rows were being written and then thrownaway — no field on any response, no realtime consumer. This exposes them.
Backend read side only. The frontend badge/panel is #654; the docs/website sync sweep is #655.
What changed
Query detail —
GET /queries/{id}gains anapproval_predictionblock.core.api.QueryDetailViewgrows anApprovalPredictionDetailnested record, populated inDefaultQueryRequestLookupServiceand mapped to the wire byQueryDetailResponse. It carriesid,probability,skipped,skipped_reason,failed,created_at— deliberately notmodel_id,feature_schema_version, the feature snapshot, or the failure message.skipped_reasonstays the machine token (
DISABLED/MODEL_NOT_SERVING) the serving path wrote; the clientlocalizes it.
Unlike the AF-624 cost estimate beside it, there is no reciprocal
query_requests.approval_prediction_idFK to short-circuit on, so this is one unconditional lookup on the
query_request_idUNIQUE index perdetail fetch. Adding an FK would need a migration this issue does not ask for and would contradict the
schema in
docs/03-data-model.md.Review queue —
GET /reviews/pendinggains a nullableapproval_probabilityper row.DefaultReviewService.listPendingForReviewernow filters the page to the actionable rows first, thenmakes one
findByQueryRequestIdscall for the whole page and keys it into aMap<UUID, Double>.A per-row lookup here would be an N+1 on every queue render;
DefaultReviewServiceTestpins thesingle-call property with
verify(..., times(1))and also covers the no-actionable-rowsshort-circuit. Sentinel rows carry no probability and are simply absent from the map.
Realtime —
RealtimeEventDispatcherconsumesApprovalPredictionCompletedEventand pushesquery.prediction_complete. Unlike the submitter-onlyquery.estimate_complete, it fans out to thereview plan's first-stage approvers (the same set
review.new_requesttargets, already excluding thesubmitter) plus the submitter, whose detail page renders the same block. The payload is a refetch
trigger, not data:
query_idand aprobabilitythat is JSON-nullon the sentinel rows.query.prediction_completeis also registered in the frontend WS contract(
WsEventName/WsEventPayloadMap/WS_EVENT_NAMES). Without that entrywebsocketManagerhard-drops every frame with a console warning, so the event would be inert until #654. That is the
wire contract only — the badge and panel remain #654's scope.
Advisory-only guarantee stays structural: three read surfaces and nothing else. The prediction is
not an input to routing, grant coverage, break-glass, or any decision path.
Docs updated
docs/04-api-spec.md— theapproval_predictionblock in the query-detailexample plus an explanatory paragraph;
approval_probabilityin the review-queue example plus aparagraph; a
query.prediction_completerow in the WebSocket events table.docs/05-backend.md— a "Read side (AF-653)" subsection under"Approval-outcome prediction", and the event added to the source-events → WS-events registry table.
Not touched, deliberately:
docs/03-data-model.md(no schema change),docs/09-deployment.md(no newenv var),
README.mdandwebsite/(the feature has no website presence yet — #655 is the dedicatedsync sweep), and
e2e/(no route, selector, form, or user-facing flow changes).Verification
mvn clean verify -Pcoverage— 5845 tests, 0 failures.mvn test -Dtest='ApplicationModulesTest,ApiPackageDependencyTest'— pass.lint(0 errors) +typecheck+test:coverage(1333 tests, 91.85% statements /83.66% branches) +
build— all green.ApprovalPredictionIntegrationTestgains two real-Postgres cases proving the detail view resolvesthe prediction without a reciprocal FK, and that it stays null before scoring.
Review notes
af-verifier,af-reviewerandaf-java-reviewerran concurrently. No Blockers from any of them.Concerns acted on in 63aa476: the "lowest open stage" doc wording, the missing WS registry row, the
frontend WS union entry, and an unreachable
Collectors.toMapmerge function. Surviving items:af-java-reviewer— the twoQueryDetailViewbackward-compatible constructors have noproduction callers and silently default the newest component, so a future caller could get
approvalPrediction == null(indistinguishable from "not scored yet") with no compile error.Not acted on: workflow: approval prediction — expose in query detail + review queue + WS event #653 explicitly specifies "Add a backward-compatible constructor delegating with
null, exactly like the existing cost-estimate constructor chain", and the 24-arg overload itmirrors is already on
main. Removing them reverts an existing decision beyond this issue's scope.Worth a separate look if the chain reaches a fourth arity.
af-reviewer— the WS recipient set is the plan's first stage, not the query's currently openstage. Exact for the first push, but the late-estimate rescore can publish a second event after a
stage-1 decision, and that one still targets stage-1 approvers. Not acted on in code: resolving
the open stage needs the decision list and the routing engine's effective
min_approvals, bothbehind
workflow-internal beans therealtimemodule cannot reach — closing it would mean a newapisurface for a refetch hint. The docs now state the actual behaviour and the reasoning.af-verifier— found a pre-existing dead anchor onmain(
#post-queriesdry-run--response-200in the AF-624 paragraph,docs/04-api-spec.md). Not from thisbranch; left for a separate docs fix.
One process note for reviewers reading the run logs: an earlier
mvn verifyreported anApiPackageDependencyTestfailure ("failed to check any classes"). That was a concurrent-Mavenartifact in my working tree — another invocation rewrote
target/classesmid-run. The cleanmvn clean verify -Pcoverageabove passes, as does the gate standalone and in a fresh worktree.