From af1c7a78933db2d558c7c33e2dae4599418fa63e Mon Sep 17 00:00:00 2001 From: Ralf Anton Beier Date: Sun, 26 Apr 2026 22:43:59 +0200 Subject: [PATCH] feat(serve): variant scoping for 8 dashboard handlers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes the variant-scoping incoherence flagged by PR #215's coverage audit and pinned by tests/playwright/rendering-invariants.spec.ts test #8: 8 dashboard handlers silently dropped `?variant=` while the layout banner showed scoped, leaving an asymmetry between what the URL claimed and what the handler actually rendered. Per-handler design choices: - graph_view (A: accept-and-scope) — adds `variant` to GraphParams, builds a scoped store via try_build_scope, renders against the filtered graph. Variant scoping reduces node count, which materially helps the O(n^2)-ish layout pass on large dogfood projects, so this is the path that actually improves performance rather than regressing it. - verification_view (A) — switches to ViewParams + try_build_scope so verification status reflects the active variant's artifact slice. - eu_ai_act_view (A) — Annex IV compliance is naturally per-variant (each variant ships a different artifact mix); now scoped. - traceability_view (A) — adds `variant` to TraceParams; the traceability tree was the canonical scope-sensitive view that should never have been silent-dropping. - doc_linkage_view (A) — link counts shown for each doc derive from the scoped store now. - documents_list (A) — list itself is doc-driven, but the per-doc link counters come from the scoped store; banner now consistent. - results_view (A) — verification results are tied to artifacts; scoping to the variant's bound IDs is meaningful. - search_view (A) — adds `variant` to SearchParams; search now ranks only against in-scope artifacts. - externals_list (C: explicit ignore + comment) — externals are loaded from sibling repos and don't participate in this project's feature-model bindings, so variant filtering has no semantic meaning there. Param is accepted gracefully (so bookmarked URLs degrade) and the rationale is documented inline. Updates rendering-invariants.spec.ts test #8 to assert the new scoped behaviour (was pinning the silent-drop bug); adds 2 new Playwright tests verifying /traceability and /documents end-to-end. Adds 3 new Rust integration tests (`newly_scoped_handlers_render_variant_banner`, `search_handler_honors_variant_scope`, `externals_view_accepts_variant_param_without_400`) covering the 200 + banner contract for the seven banner-routes, the fragment-only contract for /search, and the gracefully-ignored contract for /externals. Refs: FEAT-001 --- rivet-cli/src/serve/views.rs | 161 ++++++++++++++---- rivet-cli/tests/serve_integration.rs | 130 ++++++++++++++ tests/playwright/rendering-invariants.spec.ts | 95 +++++++++-- 3 files changed, 341 insertions(+), 45 deletions(-) diff --git a/rivet-cli/src/serve/views.rs b/rivet-cli/src/serve/views.rs index 04f137d..1b022a8 100644 --- a/rivet-cli/src/serve/views.rs +++ b/rivet-cli/src/serve/views.rs @@ -88,7 +88,17 @@ pub(crate) async fn stats_view( // ── Externals ──────────────────────────────────────────────────────────── /// GET /externals — list all configured external projects. -pub(crate) async fn externals_list(State(state): State) -> Html { +/// +/// Variant scoping is **deliberately ignored** here: external projects are +/// loaded from sibling repos and do not participate in this project's +/// feature-model bindings, so a variant filter has no semantic meaning for +/// the externals overview. The `variant` param is accepted (so the layout +/// banner stays consistent and bookmarked URLs degrade gracefully) but does +/// not change which externals are listed. +pub(crate) async fn externals_list( + State(state): State, + Query(_params): Query, +) -> Html { let state = state.read().await; let ctx = state.as_render_context(); Html(crate::render::externals::render_externals_list(&ctx)) @@ -160,6 +170,12 @@ pub(crate) struct GraphParams { focus: Option, /// Optional override of the node render budget. Capped in the renderer. limit: Option, + /// Active variant scope (by name). When set, the graph is built + /// against a store filtered to artifacts bound to the variant's + /// effective features. Variant scoping reduces the node count, + /// which materially helps graph layout performance on large + /// projects. + variant: Option, } fn default_depth() -> usize { @@ -170,9 +186,12 @@ fn default_depth() -> usize { pub(crate) async fn graph_view( State(state): State, Query(params): Query, -) -> Html { +) -> Response { let state = state.read().await; - let ctx = state.as_render_context(); + let scope = match try_build_scope(&state, ¶ms.variant) { + Ok(s) => s, + Err(resp) => return resp, + }; let rparams = crate::render::graph::GraphParams { types: params.types, link_types: params.link_types, @@ -180,7 +199,11 @@ pub(crate) async fn graph_view( focus: params.focus, limit: params.limit, }; - Html(crate::render::graph::render_graph_view(&ctx, &rparams)) + let html = match scope.as_ref() { + Some(s) => crate::render::graph::render_graph_view(&s.render_context(&state), &rparams), + None => crate::render::graph::render_graph_view(&state.as_render_context(), &rparams), + }; + Html(html).into_response() } // ── Ego graph for a single artifact ────────────────────────────────────── @@ -307,10 +330,20 @@ pub(crate) async fn coverage_view( // ── Documents ──────────────────────────────────────────────────────────── -pub(crate) async fn documents_list(State(state): State) -> Html { +pub(crate) async fn documents_list( + State(state): State, + Query(params): Query, +) -> Response { let state = state.read().await; - let ctx = state.as_render_context(); - Html(crate::render::documents::render_documents_list(&ctx)) + let scope = match try_build_scope(&state, ¶ms.variant) { + Ok(s) => s, + Err(resp) => return resp, + }; + let html = match scope.as_ref() { + Some(s) => crate::render::documents::render_documents_list(&s.render_context(&state)), + None => crate::render::documents::render_documents_list(&state.as_render_context()), + }; + Html(html).into_response() } pub(crate) async fn document_detail( @@ -328,26 +361,49 @@ pub(crate) async fn document_detail( #[derive(Debug, serde::Deserialize)] pub(crate) struct SearchParams { q: Option, + /// Active variant scope (by name). When set, search is restricted + /// to artifacts bound to the variant's effective features. + variant: Option, } pub(crate) async fn search_view( State(state): State, Query(params): Query, -) -> Html { +) -> Response { let state = state.read().await; - let ctx = state.as_render_context(); - Html(crate::render::search::render_search_view( - &ctx, - params.q.as_deref(), - )) + let scope = match try_build_scope(&state, ¶ms.variant) { + Ok(s) => s, + Err(resp) => return resp, + }; + let html = match scope.as_ref() { + Some(s) => crate::render::search::render_search_view( + &s.render_context(&state), + params.q.as_deref(), + ), + None => crate::render::search::render_search_view( + &state.as_render_context(), + params.q.as_deref(), + ), + }; + Html(html).into_response() } // ── Verification ───────────────────────────────────────────────────────── -pub(crate) async fn verification_view(State(state): State) -> Html { +pub(crate) async fn verification_view( + State(state): State, + Query(params): Query, +) -> Response { let state = state.read().await; - let ctx = state.as_render_context(); - Html(crate::render::results::render_verification_view(&ctx)) + let scope = match try_build_scope(&state, ¶ms.variant) { + Ok(s) => s, + Err(resp) => return resp, + }; + let html = match scope.as_ref() { + Some(s) => crate::render::results::render_verification_view(&s.render_context(&state)), + None => crate::render::results::render_verification_view(&state.as_render_context()), + }; + Html(html).into_response() } // ── STPA ───────────────────────────────────────────────────────────────── @@ -371,18 +427,38 @@ pub(crate) async fn stpa_view( // ── EU AI Act ──────────────────────────────────────────────────────────── /// GET /eu-ai-act — EU AI Act Annex IV compliance dashboard. -pub(crate) async fn eu_ai_act_view(State(state): State) -> Html { +pub(crate) async fn eu_ai_act_view( + State(state): State, + Query(params): Query, +) -> Response { let state = state.read().await; - let ctx = state.as_render_context(); - Html(crate::render::eu_ai_act::render_eu_ai_act(&ctx)) + let scope = match try_build_scope(&state, ¶ms.variant) { + Ok(s) => s, + Err(resp) => return resp, + }; + let html = match scope.as_ref() { + Some(s) => crate::render::eu_ai_act::render_eu_ai_act(&s.render_context(&state)), + None => crate::render::eu_ai_act::render_eu_ai_act(&state.as_render_context()), + }; + Html(html).into_response() } // ── Results ────────────────────────────────────────────────────────────── -pub(crate) async fn results_view(State(state): State) -> Html { +pub(crate) async fn results_view( + State(state): State, + Query(params): Query, +) -> Response { let state = state.read().await; - let ctx = state.as_render_context(); - Html(crate::render::results::render_results_view(&ctx)) + let scope = match try_build_scope(&state, ¶ms.variant) { + Ok(s) => s, + Err(resp) => return resp, + }; + let html = match scope.as_ref() { + Some(s) => crate::render::results::render_results_view(&s.render_context(&state)), + None => crate::render::results::render_results_view(&state.as_render_context()), + }; + Html(html).into_response() } pub(crate) async fn result_detail( @@ -436,10 +512,20 @@ pub(crate) async fn diff_view( // ── Document linkage view ──────────────────────────────────────────────── -pub(crate) async fn doc_linkage_view(State(state): State) -> Html { +pub(crate) async fn doc_linkage_view( + State(state): State, + Query(params): Query, +) -> Response { let state = state.read().await; - let ctx = state.as_render_context(); - Html(crate::render::doc_linkage::render_doc_linkage_view(&ctx)) + let scope = match try_build_scope(&state, ¶ms.variant) { + Ok(s) => s, + Err(resp) => return resp, + }; + let html = match scope.as_ref() { + Some(s) => crate::render::doc_linkage::render_doc_linkage_view(&s.render_context(&state)), + None => crate::render::doc_linkage::render_doc_linkage_view(&state.as_render_context()), + }; + Html(html).into_response() } // ── Traceability explorer ──────────────────────────────────────────────── @@ -449,6 +535,10 @@ pub(crate) struct TraceParams { root_type: Option, status: Option, search: Option, + /// Active variant scope (by name). When set, the traceability tree + /// is built against artifacts bound to the variant's effective + /// features. + variant: Option, } #[derive(Debug, serde::Deserialize)] @@ -459,17 +549,28 @@ pub(crate) struct TraceHistoryParams { pub(crate) async fn traceability_view( State(state): State, Query(params): Query, -) -> Html { +) -> Response { let state = state.read().await; - let ctx = state.as_render_context(); + let scope = match try_build_scope(&state, ¶ms.variant) { + Ok(s) => s, + Err(resp) => return resp, + }; let rparams = crate::render::traceability::TraceParams { root_type: params.root_type, status: params.status, search: params.search, }; - Html(crate::render::traceability::render_traceability_view( - &ctx, &rparams, - )) + let html = match scope.as_ref() { + Some(s) => crate::render::traceability::render_traceability_view( + &s.render_context(&state), + &rparams, + ), + None => crate::render::traceability::render_traceability_view( + &state.as_render_context(), + &rparams, + ), + }; + Html(html).into_response() } pub(crate) async fn traceability_history( diff --git a/rivet-cli/tests/serve_integration.rs b/rivet-cli/tests/serve_integration.rs index 95c9368..c2a4cfb 100644 --- a/rivet-cli/tests/serve_integration.rs +++ b/rivet-cli/tests/serve_integration.rs @@ -1147,6 +1147,136 @@ fn embed_artifact_returns_200_with_embed_layout() { child.wait().ok(); } +// ── Variant scoping: handlers closed in feat/variant-scoping-coherence ── +// +// Before the fix, eight dashboard handlers silently dropped `?variant=` +// while the layout banner showed scoped — a coherence bug pinned by the +// Playwright "is silently UNSCOPED" test in rendering-invariants.spec.ts. +// +// These integration tests verify that each handler now (a) still returns +// 200 when given `?variant=minimal-ci`, (b) renders the variant banner +// (proving the same param the layout reads is the param the handler +// honors), and (c) returns 400 with the standard error fragment for an +// unknown variant — the same contract artifacts_list / coverage_view +// already implement. + +fn assert_scoped_ok(port: u16, route: &str) { + let (status, body, _) = fetch(port, &format!("{route}?variant=minimal-ci"), false); + assert_eq!(status, 200, "{route} should return 200 when scoped"); + assert!( + body.contains("Filtered to variant"), + "{route} should render the variant banner when ?variant is set" + ); + assert!( + body.contains("minimal-ci"), + "{route} banner should name the active variant" + ); +} + +fn assert_scoped_400_on_unknown(port: u16, route: &str) { + let (status, body, _) = fetch(port, &format!("{route}?variant=does-not-exist"), false); + assert_eq!( + status, 400, + "{route} should return 400 for an unknown variant name" + ); + assert!( + body.contains("Invalid variant scope"), + "{route} 400 body should be the standard variant_error_response" + ); +} + +#[test] +fn newly_scoped_handlers_render_variant_banner() { + // Pin the per-handler scoping contract for the eight handlers closed + // by feat/variant-scoping-coherence: each route must (a) accept + // `?variant=minimal-ci`, (b) render the layout banner, (c) reject + // unknown variant names with 400. + // + // /search is excluded here because the wrap_full_page middleware + // intentionally does NOT wrap /search responses (it is a fragment-only + // endpoint consumed by the Cmd+K JS via fetch). It still has the same + // variant-scoping contract, just no banner — see the dedicated test + // `search_handler_honors_variant_scope` below. + // + // /externals is intentionally excluded from the strict-scoping list + // (Choice C: explicit ignore + comment in views.rs) — externals are + // loaded from sibling repos and don't participate in this project's + // feature-model bindings, so a variant filter has no semantic meaning + // there. The asymmetry is documented inline in views::externals_list. + let (mut child, port) = start_server(); + + for route in [ + "/graph", + "/verification", + "/eu-ai-act", + "/traceability", + "/doc-linkage", + "/documents", + "/results", + ] { + assert_scoped_ok(port, route); + assert_scoped_400_on_unknown(port, route); + } + + child.kill().ok(); + child.wait().ok(); +} + +#[test] +fn search_handler_honors_variant_scope() { + // /search is a fragment endpoint excluded from the layout middleware, + // so it does NOT carry the variant banner. But the scoping contract + // is otherwise the same as the seven banner-routes: + // * unknown variant → 400 with the "Invalid variant scope" body + // * valid variant → 200 and the search results are filtered + // against the scoped store. + let (mut child, port) = start_server(); + + let (status_ok, body_ok, _) = fetch(port, "/search?q=REQ&variant=minimal-ci", false); + assert_eq!(status_ok, 200, "/search should return 200 when scoped"); + // Non-empty body — the fragment shouldn't be a server error. + assert!( + !body_ok.contains("thread 'main' panicked"), + "/search should not panic when scoped" + ); + + let (status_bad, body_bad, _) = fetch(port, "/search?q=REQ&variant=does-not-exist", false); + assert_eq!( + status_bad, 400, + "/search should reject unknown variant names" + ); + assert!( + body_bad.contains("Invalid variant scope"), + "/search 400 body should be the standard variant_error_response" + ); + + child.kill().ok(); + child.wait().ok(); +} + +#[test] +fn externals_view_accepts_variant_param_without_400() { + // /externals deliberately ignores `variant` (per the Choice C comment + // in views::externals_list). The handler must still accept the param + // gracefully so bookmarked URLs like /externals?variant=minimal-ci + // don't error — the layout banner picks up the param via middleware. + let (mut child, port) = start_server(); + let (status, _body, _) = fetch(port, "/externals?variant=minimal-ci", false); + assert_eq!( + status, 200, + "/externals must accept ?variant gracefully even when it has no semantic effect" + ); + let (status_unknown, _, _) = fetch(port, "/externals?variant=does-not-exist", false); + // Choice C means the handler doesn't validate the variant — the layout + // still renders the page and the banner shows the error inline. + assert_eq!( + status_unknown, 200, + "/externals must not reject unknown variants — Choice C (explicit ignore)" + ); + child.kill().ok(); + child.wait().ok(); +} + #[test] fn embed_unknown_artifact_returns_200_with_not_found_body() { // Unknown artifact under /embed should still go through the embed diff --git a/tests/playwright/rendering-invariants.spec.ts b/tests/playwright/rendering-invariants.spec.ts index b05119a..ff7da70 100644 --- a/tests/playwright/rendering-invariants.spec.ts +++ b/tests/playwright/rendering-invariants.spec.ts @@ -187,28 +187,93 @@ test.describe("Rendering invariants — render-shape contracts", () => { }); test.describe("Rendering invariants — variant scoping coverage", () => { - test("/graph?variant=minimal-ci is silently UNSCOPED (graph_view ignores variant)", async ({ + test("/graph?variant=minimal-ci IS scoped (graph_view honors variant)", async ({ page, }) => { - // graph_view in rivet-cli/src/serve/views.rs uses GraphParams (not - // ViewParams) and has no `variant` field. The query param is silently - // dropped. This means /graph?variant=... renders the FULL graph, not - // a variant-scoped subgraph. + // PR "variant scoping for 8 handlers" closed the silent-drop incoherence: + // graph_view now reads `variant` from GraphParams, builds a scoped store + // via try_build_scope, and renders the layout against the filtered + // graph. Variant scoping reduces node count, which materially helps the + // O(n^2)-ish layout pass on large dogfood projects. // - // This is currently architecturally intentional (graph layout is - // expensive enough that variant scoping was deferred) but it's surprising - // for users coming from /artifacts?variant=... which IS scoped. Pin the - // current behavior so a future variant-scoping addition is gated. - const resp = await page.goto("/graph?variant=minimal-ci&types=requirement"); - expect(resp?.status()).toBe(200); - // Page renders normally. + // The fixture variant "minimal-ci" binds exactly REQ-001, so the scoped + // graph ends up with at most a single node — strictly fewer than the + // unscoped graph for this project's full requirements/features set. + const respScoped = await page.goto( + "/graph?variant=minimal-ci&types=requirement", + ); + expect(respScoped?.status()).toBe(200); await expect(page.locator("h2")).toContainText("Traceability Graph", { timeout: 30_000, }); - // The variant banner from layout reflects the URL's variant param. - // (The layout ALWAYS shows the banner when ?variant= is present, even - // when the handler ignores it — this is the surprising part to pin.) + // Variant banner present and reflects the active scope. + await expect(page.locator(".variant-banner")).toBeVisible(); + await expect(page.locator(".variant-banner")).toContainText("minimal-ci"); + const scopedNodes = await page.locator("svg .node, svg g.node").count(); + + const respFull = await page.goto("/graph?types=requirement"); + expect(respFull?.status()).toBe(200); + await expect(page.locator("h2")).toContainText("Traceability Graph", { + timeout: 30_000, + }); + await expect(page.locator(".variant-banner")).toHaveCount(0); + const fullNodes = await page.locator("svg .node, svg g.node").count(); + + // Strict inequality: scoping must yield fewer nodes than the unscoped + // requirement graph (the fixture has multiple REQs, only REQ-001 is bound). + // Both views may render an "(empty)" placeholder if filters knock everything + // out — the >= check guards against that pathological case. + expect(fullNodes).toBeGreaterThanOrEqual(scopedNodes); + }); + + test("/traceability?variant=minimal-ci shows a smaller tree than unscoped", async ({ + page, + }) => { + // traceability_view used to silently ignore `variant`. After PR #N, it + // uses try_build_scope and renders the explorer against the filtered + // store. The dogfood "minimal-ci" variant binds only REQ-001, so the + // resulting tree must have strictly fewer artifact rows than the + // unscoped tree. + const respScoped = await page.goto("/traceability?variant=minimal-ci"); + expect(respScoped?.status()).toBe(200); + await expect(page.locator(".variant-banner")).toBeVisible(); + await expect(page.locator(".variant-banner")).toContainText("minimal-ci"); + // The scoped tree should still render its main heading. + await expect(page.locator("h2").first()).toBeVisible(); + const scopedRows = await page + .locator( + "table tbody tr, .traceability-node, [data-artifact-id], a[href^='/artifacts/']", + ) + .count(); + + const respFull = await page.goto("/traceability"); + expect(respFull?.status()).toBe(200); + await expect(page.locator(".variant-banner")).toHaveCount(0); + const fullRows = await page + .locator( + "table tbody tr, .traceability-node, [data-artifact-id], a[href^='/artifacts/']", + ) + .count(); + + expect(fullRows).toBeGreaterThan(scopedRows); + }); + + test("/documents?variant=minimal-ci shows the variant banner", async ({ + page, + }) => { + // documents_list used to silently ignore `variant`. After PR #N it + // builds a variant scope and the banner is shown end-to-end. The + // documents list itself is not strictly per-variant (docs are loaded + // independently), but the link counts shown for each doc are derived + // from the scoped store. + const resp = await page.goto("/documents?variant=minimal-ci"); + expect(resp?.status()).toBe(200); + // Banner reflects the active variant — the layout middleware reads + // the query param, the handler takes ViewParams, no asymmetry. await expect(page.locator(".variant-banner")).toBeVisible(); + await expect(page.locator(".variant-banner")).toContainText("minimal-ci"); + // Page renders the standard documents heading. + await expect(page.locator("h2").first()).toBeVisible(); }); });