From ae4121b7c12fef696de41beec7b4c6e20d54aa55 Mon Sep 17 00:00:00 2001 From: Ragnor Comerford Date: Wed, 27 May 2026 18:32:30 +0200 Subject: [PATCH] mr-668: close coverage gaps surfaced by the test-coverage audit MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The bot-review pass and the subsequent coverage audit surfaced two material gaps in PR #119's test surface — both easy to close, both worth closing before merge. * **Gap 1 — cluster-route sweep.** The Bug-1 path-extractor regression slipped through because `cluster_routes_dispatch_per_graph_handle` only exercised `/snapshot`. The other six protected cluster routes (`/read`, `/change`, `/export`, `/schema`, `/schema/apply`, `/ingest`, `/branches/merge`) were implicitly trusted to work without any multi-mode integration test. Add `all_protected_cluster_routes_resolve_to_their_handler` (`tests/server.rs`) that hits each protected cluster route with a minimal request and asserts the response is consistent with the handler being reached — no 404 (router didn't match), no 500 with "Wrong number of path arguments" (Bug-1 class), no 500 with "missing extension" (routing middleware didn't inject the handle). Status code is a negative assertion because each handler's happy-path inputs differ; what matters is "the request reached the handler," not "the handler returned 200" — that's already pinned by the single-mode tests. * **Gap 2 — `--force` happy path.** The strict re-init regression test (`init_on_existing_graph_uri_does_not_destroy_existing_schema`) pins the error path; nothing pinned the `force: true` escape hatch actually doing what its docstring claims. Add `init_with_force_recovers_from_orphan_schema_files` (`tests/lifecycle.rs`). Writes a bare `_schema.pg` to simulate orphan files from a failed prior init, confirms strict mode bails as expected, then confirms `init_with_options(force: true)` succeeds and produces a functional graph. Note: the test follows the documented semantics — force skips the preflight only, it does NOT purge existing Lance state. An earlier draft of the test (against full overwrite of an existing populated graph) failed because `GraphCoordinator::init` errored on the existing `__manifest`, which is exactly the limitation the `InitOptions::force` docstring already calls out. Recursive purge needs `StorageAdapter::delete_prefix` (tracked separately). Coverage is now fully aligned with the PR's claims. Co-Authored-By: Claude Opus 4.7 (1M context) --- crates/omnigraph-server/tests/server.rs | 82 ++++++++++++++++++++++++- crates/omnigraph/tests/lifecycle.rs | 55 ++++++++++++++++- 2 files changed, 133 insertions(+), 4 deletions(-) diff --git a/crates/omnigraph-server/tests/server.rs b/crates/omnigraph-server/tests/server.rs index 39eca40..95c9537 100644 --- a/crates/omnigraph-server/tests/server.rs +++ b/crates/omnigraph-server/tests/server.rs @@ -4482,6 +4482,81 @@ mod multi_graph_startup { assert_eq!(resp.status(), StatusCode::NOT_FOUND); } + /// Coverage net for cluster-route regressions across every + /// protected handler — not just the few that have inner path + /// params. Bug-1 surfaced because only `/snapshot` was being + /// exercised in cluster mode, leaving the other six protected + /// routes implicitly untested. This sweep hits each one and + /// asserts the response shows the handler was reached: no 404 + /// (router didn't match), no 500 with "Wrong number of path + /// arguments" (path extractor broke), no 500 with "missing + /// extension" (routing middleware didn't inject the handle). + /// + /// Status codes are negative assertions because each handler's + /// happy-path inputs differ — what matters is "the request + /// reached the handler," not "the handler returned 200." The + /// individual handlers' logic is already tested in single mode. + #[tokio::test(flavor = "multi_thread")] + async fn all_protected_cluster_routes_resolve_to_their_handler() { + let (_dirs, app) = build_multi_mode_app(&["alpha"]).await; + + // (method, path, body) — one minimal request per protected + // cluster route. Bodies are valid enough that the router and + // extractors succeed; whether the engine ultimately returns + // 200 or 4xx is per-handler and not what this test pins. + let cases: &[(Method, &str, Option<&str>)] = &[ + (Method::GET, "/graphs/alpha/snapshot?branch=main", None), + (Method::GET, "/graphs/alpha/schema", None), + (Method::GET, "/graphs/alpha/branches", None), + (Method::GET, "/graphs/alpha/commits", None), + (Method::POST, "/graphs/alpha/read", Some(r#"{"query_source":"query q() { return {} }"}"#)), + (Method::POST, "/graphs/alpha/change", Some(r#"{"query_source":"query q() { return {} }"}"#)), + (Method::POST, "/graphs/alpha/export", Some(r#"{"branch":"main"}"#)), + (Method::POST, "/graphs/alpha/schema/apply", Some(r#"{"schema_source":"","allow_data_loss":false}"#)), + (Method::POST, "/graphs/alpha/ingest", Some(r#"{"data":""}"#)), + (Method::POST, "/graphs/alpha/branches/merge", Some(r#"{"source":"main","target":"main"}"#)), + ]; + + for (method, path, body) in cases { + let req_body = body.map(|s| Body::from(s.to_string())).unwrap_or_else(Body::empty); + let req = Request::builder() + .method(method.clone()) + .uri(*path) + .header("content-type", "application/json") + .body(req_body) + .unwrap(); + let resp = app.clone().oneshot(req).await.unwrap(); + let status = resp.status(); + let bytes = to_bytes(resp.into_body(), usize::MAX).await.unwrap(); + let body_str = String::from_utf8_lossy(&bytes); + + assert_ne!( + status, + StatusCode::NOT_FOUND, + "{} {} — router didn't match (cluster-route mounting regression). Body: {}", + method, + path, + body_str, + ); + assert!( + !(status == StatusCode::INTERNAL_SERVER_ERROR + && body_str.contains("Wrong number of path arguments")), + "{} {} — path extractor broke (Bug-1 class regression). Body: {}", + method, + path, + body_str, + ); + assert!( + !(status == StatusCode::INTERNAL_SERVER_ERROR + && body_str.to_lowercase().contains("missing extension")), + "{} {} — routing middleware didn't inject GraphHandle. Body: {}", + method, + path, + body_str, + ); + } + } + /// Regression for the bot-surfaced path-extractor bug: cluster /// routes whose inner path also captures a parameter /// (`/graphs/{graph_id}/branches/{branch}`, @@ -4493,9 +4568,10 @@ mod multi_graph_startup { /// GET commit-by-id break in multi-mode because their handlers /// use bare `Path` — this test pins the fix. /// - /// `cluster_routes_dispatch_per_graph_handle` only exercises - /// `/snapshot` (no Path extractor), so the regression slipped - /// through. This test closes that gap structurally. + /// The broader `all_protected_cluster_routes_resolve_to_their_handler` + /// test sweeps the full route surface; this one stays narrowly + /// targeted at the inner-path-param shape because that's the + /// specific regression class. #[tokio::test(flavor = "multi_thread")] async fn cluster_routes_with_inner_path_params_deserialize_correctly() { let (_dirs, app) = build_multi_mode_app(&["alpha"]).await; diff --git a/crates/omnigraph/tests/lifecycle.rs b/crates/omnigraph/tests/lifecycle.rs index 399142e..a56a80c 100644 --- a/crates/omnigraph/tests/lifecycle.rs +++ b/crates/omnigraph/tests/lifecycle.rs @@ -2,7 +2,7 @@ mod helpers; use std::fs; -use omnigraph::db::{Omnigraph, ReadTarget}; +use omnigraph::db::{InitOptions, Omnigraph, ReadTarget}; use omnigraph_compiler::schema::parser::parse_schema; use omnigraph_compiler::{build_schema_ir, schema_ir_pretty_json}; @@ -251,3 +251,56 @@ async fn init_on_existing_graph_uri_does_not_destroy_existing_schema() { "__schema_state.json contents must be preserved when re-init is rejected" ); } + +/// Happy-path sibling to the strict re-init regression above: +/// `InitOptions { force: true }` must skip the schema-file preflight +/// when the operator deliberately wants to recover from orphan +/// schema artifacts (e.g. files left behind by a failed prior init). +/// +/// Documented semantics per `InitOptions::force`: skips the preflight +/// only. Force does NOT purge existing Lance datasets or `__manifest/` +/// — that needs `StorageAdapter::delete_prefix`, which is tracked +/// separately. The realistic recovery scenario is "schema files +/// exist but Lance state doesn't," which this test reproduces. +/// +/// Without this test, a future refactor could invert the `if !force` +/// branch and silently break the operator-facing escape hatch. +#[tokio::test] +async fn init_with_force_recovers_from_orphan_schema_files() { + let dir = tempfile::tempdir().unwrap(); + let uri = dir.path().to_str().unwrap(); + + // Simulate orphan schema files: write `_schema.pg` to disk + // without running a full init. The preflight will see it and + // bail in strict mode. + fs::write(dir.path().join("_schema.pg"), TEST_SCHEMA).unwrap(); + + // Strict mode refuses because `_schema.pg` exists. + let strict_err = match Omnigraph::init(uri, TEST_SCHEMA).await { + Ok(_) => panic!("strict init must refuse when orphan _schema.pg exists"), + Err(e) => e, + }; + assert!( + strict_err.to_string().contains("already initialized"), + "strict init must surface AlreadyInitialized (sanity check); got: {strict_err}" + ); + + // Force init succeeds: it skips the preflight, overwrites the + // orphan file, and proceeds to initialize Lance state (which + // didn't exist, so `GraphCoordinator::init` is unblocked). + let db = Omnigraph::init_with_options(uri, TEST_SCHEMA, InitOptions { force: true }) + .await + .expect("force init must succeed when only orphan schema files block strict init"); + + // Confirm the catalog is populated as expected — proves the + // graph is functional after force-recovery, not just that the + // call returned Ok. + assert!( + db.catalog().node_types.contains_key("Person"), + "force-recovered graph must have the new catalog installed" + ); + assert!( + dir.path().join("__schema_state.json").exists(), + "force-recovered graph must have full schema state written" + ); +}