diff --git a/docs/explanation/branching.md b/docs/explanation/branching.md index 3e55eec2..80094626 100644 --- a/docs/explanation/branching.md +++ b/docs/explanation/branching.md @@ -129,6 +129,11 @@ Detection alone is half a design; this section is the other half. merge gate always runs the dry-run overlay validation, even for "clean" merges. +A dry run reports the same gate without landing anything: the landable +entries split into added, changed and deleted, and every finding the merged +world carries, errors and non-blocking ones alike. A reviewer reads one +result instead of a diff and a separate validation call. + **Resolution happens on the branch, never inside the merge.** Merge is all-or-nothing: it either lands everything or reports conflicts and changes nothing. Resolving is ordinary branch work, not a special mode: diff --git a/docs/how-to/repair-validation-findings.md b/docs/how-to/repair-validation-findings.md index 22d84cf5..a1724252 100644 --- a/docs/how-to/repair-validation-findings.md +++ b/docs/how-to/repair-validation-findings.md @@ -37,6 +37,12 @@ clarification. A new rule or unclear result can require inspection of its validator implementation; the catalog and tool contracts are the starting point. +To survey a whole namespace instead, run `validate_project` with `namespace` +set to it and `json: true`: every EventModel in the namespace is validated in +one call. Filter its findings on a code to list what needs work, for example +`SLICE_NO_SCENARIOS` (Info) for the slices that still lack a given/when/then +scenario. + ## Discover the full dependency context Use existing model tools before searching exports manually: @@ -147,6 +153,7 @@ errors and `isError` before reading that envelope. | `get_entity_json` | `{ "etag": "...", "json": { ... } }` with an object-valued entity. | | `validate_event_model` with `json: true` | `{ "json": "..." }` with a JSON-encoded response string. | | Reference tools, `get_impact`, `batch_mutate_json` | `{ "binpb_base64": "..." }` with the protobuf response. | +| `put_entity`, `delete_entity`, `batch_mutate`, `batch_mutate_json` | `{ "scope": "branch" or "baseline", "branch": "..." or null, "binpb_base64": "..." }`. Use `scope` and `branch` to confirm which store took the write. | The JSON response helper can fall back to `binpb_base64` if reflection rendering fails. Decode binary responses using the matching RPC response diff --git a/proto/trogonatlas/api/eventmodel/v1alpha1/service.proto b/proto/trogonatlas/api/eventmodel/v1alpha1/service.proto index bc4a6c0a..d2f85888 100644 --- a/proto/trogonatlas/api/eventmodel/v1alpha1/service.proto +++ b/proto/trogonatlas/api/eventmodel/v1alpha1/service.proto @@ -1033,11 +1033,23 @@ message MergeBranchResponse { } Status status = 1; repeated trogonatlas.catalog.v1alpha1.BranchDiffEntry conflicts = 2; + // Post-merge validation findings. STATUS_INVALID carries the findings that + // blocked the merge. A dry run that reports STATUS_VALIDATED also carries + // its Warning and Info findings, so a reviewer sees what the merge leaves + // behind without a separate validation call. repeated ValidationIssue validation = 3; - // Number of non-CONVERGED entries actually landed on baseline. + // Number of non-CONVERGED entries actually landed on baseline, or that a + // dry run would land. uint32 applied_count = 4; // Set only when the request carried an operation_id. trogonatlas.catalog.v1alpha1.OperationReceipt operation_receipt = 5; + // The landed (or, on a dry run, landable) entries split by diff class. + // Together they sum to applied_count when the merge reached the landing + // step (STATUS_APPLIED, STATUS_VALIDATED, or a dry run's STATUS_INVALID); + // all zero for STATUS_CONFLICTS and for an operation_id replay. + uint32 added_count = 6; + uint32 changed_count = 7; + uint32 deleted_count = 8; } message UpdateBranchRequest { diff --git a/proto/trogonatlas/api/eventmodel/v1alpha1/validation.proto b/proto/trogonatlas/api/eventmodel/v1alpha1/validation.proto index 64c7d589..a61ff99f 100644 --- a/proto/trogonatlas/api/eventmodel/v1alpha1/validation.proto +++ b/proto/trogonatlas/api/eventmodel/v1alpha1/validation.proto @@ -71,18 +71,19 @@ message ListValidationRulesResponse { repeated ValidationRule rules = 1; } -// Validate every EventModel in a project (or in the entire store) in one +// Validate every EventModel in a project, a domain or a namespace in one // pass, instead of N round trips through ValidateEventModel. The // validator already has the full entity store in memory per call; // looping per-model is pure waste. message ValidateProjectRequest { - // Scope by Project.id (if your store carries Projects) or by - // Domain.id (legacy). One of the two must be set. ENTITY_KIND_UNSPECIFIED - // for the full store works only on small dev stores; large stores - // should always scope. + // Scope by Project.id (if your store carries Projects), by Domain.id + // (legacy) or by namespace. Exactly one must be set. oneof scope { trogonatlas.type.v1alpha1.Id project_id = 1; trogonatlas.type.v1alpha1.Id domain_id = 2; + // Every EventModel whose id carries this namespace, whether or not a + // Domain claims it. + string namespace = 3; } } diff --git a/rsworkspace/crates/trogon-atlas-cli/src/main.rs b/rsworkspace/crates/trogon-atlas-cli/src/main.rs index ec84f1d9..b76f20af 100644 --- a/rsworkspace/crates/trogon-atlas-cli/src/main.rs +++ b/rsworkspace/crates/trogon-atlas-cli/src/main.rs @@ -251,7 +251,8 @@ enum BranchCmd { Merge { name: String, /// Run full conflict detection and post-merge validation without - /// persisting anything, even on success. + /// persisting anything, even on success. Prints how many entities + /// would be added, changed and deleted, and the post-merge findings. #[arg(long)] dry_run: bool, /// Keep the branch (now empty of deltas) after a successful merge @@ -970,19 +971,42 @@ async fn run( if text { println!("{name} merged ({} entities applied)", resp.applied_count); } - let data = - json!({"status": "applied", "applied_count": resp.applied_count}); + let counts = branch::MergeCounts::of(&resp); + let data = json!({ + "status": "applied", + "applied_count": resp.applied_count, + "added_count": counts.added, + "changed_count": counts.changed, + "deleted_count": counts.deleted, + }); Ok((ExitCode::SUCCESS, Some(data))) } Status::Validated => { + let counts = branch::MergeCounts::of(&resp); + let severities = branch::SeverityCounts::of(&resp.validation); if text { println!( - "{name} would merge cleanly ({} entities, dry run)", + "{name} would merge cleanly (dry run): {} entities ({counts})", resp.applied_count ); + println!("validation after merge: {severities}"); + for issue in &resp.validation { + println!("{}", apply::format_issue(issue)); + } } - let data = - json!({"status": "validated", "applied_count": resp.applied_count}); + let data = json!({ + "status": "validated", + "applied_count": resp.applied_count, + "added_count": counts.added, + "changed_count": counts.changed, + "deleted_count": counts.deleted, + "validation_summary": { + "errors": severities.errors, + "warnings": severities.warnings, + "info": severities.infos, + }, + "validation": resp.validation.iter().map(apply::issue_to_json).collect::>(), + }); Ok((ExitCode::SUCCESS, Some(data))) } Status::Conflicts => { @@ -995,14 +1019,26 @@ async fn run( Ok((ExitCode::from(EXIT_FINDINGS), Some(data))) } Status::Invalid => { + let counts = branch::MergeCounts::of(&resp); + let severities = branch::SeverityCounts::of(&resp.validation); if text { eprintln!("{name} would leave baseline invalid, nothing merged:"); + eprintln!("would have landed: {counts}"); + eprintln!("validation after merge: {severities}"); for issue in &resp.validation { eprintln!("{}", apply::format_issue(issue)); } } let data = json!({ "status": "invalid", + "added_count": counts.added, + "changed_count": counts.changed, + "deleted_count": counts.deleted, + "validation_summary": { + "errors": severities.errors, + "warnings": severities.warnings, + "info": severities.infos, + }, "validation": resp.validation.iter().map(apply::issue_to_json).collect::>(), }); Ok((ExitCode::from(EXIT_FINDINGS), Some(data))) diff --git a/rsworkspace/crates/trogon-atlas-cli/tests/branch_cli_integration.rs b/rsworkspace/crates/trogon-atlas-cli/tests/branch_cli_integration.rs index fa932db0..67451c8e 100644 --- a/rsworkspace/crates/trogon-atlas-cli/tests/branch_cli_integration.rs +++ b/rsworkspace/crates/trogon-atlas-cli/tests/branch_cli_integration.rs @@ -206,6 +206,35 @@ spec: { title: On branch } "{:?}", merged.stdout ); + assert!( + merged.stdout.contains("1 added, 0 changed, 0 deleted"), + "{:?}", + merged.stdout + ); + assert!( + merged + .stdout + .contains("validation after merge: 0 errors, 0 warnings, 0 info"), + "{:?}", + merged.stdout + ); + + let as_json = trogon_atlas( + &endpoint, + &[ + "--format", + "json", + "branch", + "merge", + "cli-dry", + "--dry-run", + ], + ); + let doc: serde_json::Value = serde_json::from_str(&as_json.stdout).unwrap(); + assert_eq!(doc["data"]["added_count"], 1, "{doc}"); + assert_eq!(doc["data"]["changed_count"], 0, "{doc}"); + assert_eq!(doc["data"]["deleted_count"], 0, "{doc}"); + assert_eq!(doc["data"]["validation_summary"]["errors"], 0, "{doc}"); // Dry run must not persist: baseline still lacks the entity. let mut client = trogon_atlas_client::client::connect(&endpoint, None, 30) @@ -224,6 +253,80 @@ spec: { title: On branch } assert!(missing.is_err(), "dry run must not persist"); } +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn trogon_atlas_branch_merge_dry_run_invalid_reports_counts_and_findings() { + let endpoint = start_server().await; + + let created = trogon_atlas(&endpoint, &["branch", "create", "cli-inv"]); + assert!(created.status.success(), "{:?}", created.stderr); + + let dir = tempfile_dir(); + let manifest_path = dir.path().join("invalid.yaml"); + std::fs::write( + &manifest_path, + r" +apiVersion: eventmodel.atlas.trogonstack.com/v1alpha1 +kind: Term +metadata: { namespace: cli-inv-ns, name: ghost-term } +spec: + title: Ghost + embodiedBy: + - kind: ENTITY_KIND_EVENT + id: { namespace: cli-inv-ns, slug: does-not-exist, version: '1' } +", + ) + .unwrap(); + let applied = trogon_atlas_branched( + &endpoint, + "cli-inv", + &["apply", "-f", manifest_path.to_str().unwrap()], + ); + assert!(applied.status.success(), "{:?}", applied.stderr); + + let merged = trogon_atlas(&endpoint, &["branch", "merge", "cli-inv", "--dry-run"]); + assert_eq!(merged.status.code(), Some(1), "{:?}", merged.stderr); + assert!( + merged + .stderr + .contains("would have landed: 1 added, 0 changed, 0 deleted"), + "{:?}", + merged.stderr + ); + assert!( + merged.stderr.contains("validation after merge: 1 errors"), + "{:?}", + merged.stderr + ); + assert!( + merged.stderr.contains("TERM_DANGLING_ENTITY"), + "{:?}", + merged.stderr + ); + + let as_json = trogon_atlas( + &endpoint, + &[ + "--format", + "json", + "branch", + "merge", + "cli-inv", + "--dry-run", + ], + ); + let doc: serde_json::Value = serde_json::from_str(&as_json.stdout).unwrap(); + assert_eq!(doc["data"]["status"], "invalid", "{doc}"); + assert_eq!(doc["data"]["added_count"], 1, "{doc}"); + assert_eq!(doc["data"]["validation_summary"]["errors"], 1, "{doc}"); + let finding = doc["data"]["validation"] + .as_array() + .unwrap() + .iter() + .find(|i| i["code"] == "TERM_DANGLING_ENTITY") + .unwrap_or_else(|| panic!("{doc}")); + assert_eq!(finding["severity"], "error", "{doc}"); +} + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] async fn trogon_atlas_branch_merge_clean_exits_0() { let endpoint = start_server().await; diff --git a/rsworkspace/crates/trogon-atlas-client/src/apply.rs b/rsworkspace/crates/trogon-atlas-client/src/apply.rs index 6b73c6e3..3cf52d31 100644 --- a/rsworkspace/crates/trogon-atlas-client/src/apply.rs +++ b/rsworkspace/crates/trogon-atlas-client/src/apply.rs @@ -228,6 +228,7 @@ pub fn issue_to_json(issue: &pb::ValidationIssue) -> Value { Some(pb::canonical::id_string(kind, id)) }); json!({ + "severity": severity_name(issue), "code": issue.code, "message": issue.message, "subject": subject, @@ -380,13 +381,17 @@ pub fn render_diff(plan: &[PlannedChange], types: &TranscodePool) -> Result<(Str Ok((out, changed)) } -pub fn format_issue(issue: &pb::ValidationIssue) -> String { - let severity = match pb::validation_issue::Severity::try_from(issue.severity) { +fn severity_name(issue: &pb::ValidationIssue) -> &'static str { + match pb::validation_issue::Severity::try_from(issue.severity) { Ok(pb::validation_issue::Severity::Error) => "error", Ok(pb::validation_issue::Severity::Warning) => "warning", Ok(pb::validation_issue::Severity::Info) => "info", _ => "issue", - }; + } +} + +pub fn format_issue(issue: &pb::ValidationIssue) -> String { + let severity = severity_name(issue); let subject = issue .subject .as_ref() @@ -499,11 +504,13 @@ mod tests { issues: vec![pb::ValidationIssue { code: "ENTITY_REFERENCED".into(), message: "entity is referenced".into(), + severity: pb::validation_issue::Severity::Error as i32, ..Default::default() }], }; let json = rejected.to_json(); assert_eq!(json["failed"], "event:shop/order.placed@1"); assert_eq!(json["issues"][0]["code"], "ENTITY_REFERENCED"); + assert_eq!(json["issues"][0]["severity"], "error"); } } diff --git a/rsworkspace/crates/trogon-atlas-client/src/branch.rs b/rsworkspace/crates/trogon-atlas-client/src/branch.rs index 1c6cc45c..cdad5144 100644 --- a/rsworkspace/crates/trogon-atlas-client/src/branch.rs +++ b/rsworkspace/crates/trogon-atlas-client/src/branch.rs @@ -109,6 +109,67 @@ pub struct MergeOptions { pub auto_merge: bool, } +/// How many entries a merge lands, split by diff class. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub struct MergeCounts { + pub added: u32, + pub changed: u32, + pub deleted: u32, +} + +impl MergeCounts { + pub fn of(resp: &pb::MergeBranchResponse) -> Self { + Self { + added: resp.added_count, + changed: resp.changed_count, + deleted: resp.deleted_count, + } + } +} + +impl std::fmt::Display for MergeCounts { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + write!( + f, + "{} added, {} changed, {} deleted", + self.added, self.changed, self.deleted + ) + } +} + +/// Validation findings grouped by severity. +#[derive(Debug, Clone, Copy, PartialEq, Eq, Default)] +pub struct SeverityCounts { + pub errors: usize, + pub warnings: usize, + pub infos: usize, +} + +impl SeverityCounts { + pub fn of(issues: &[pb::ValidationIssue]) -> Self { + let mut counts = Self::default(); + for issue in issues { + match pb::validation_issue::Severity::try_from(issue.severity) { + Ok(pb::validation_issue::Severity::Error) => counts.errors += 1, + Ok(pb::validation_issue::Severity::Warning) => counts.warnings += 1, + Ok(pb::validation_issue::Severity::Info) => counts.infos += 1, + _ => {} + } + } + counts + } +} + +impl std::fmt::Display for SeverityCounts { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + write!( + f, + "{} errors, {} warnings, {} info", + self.errors, self.warnings, self.infos + ) + } +} + /// Merge a branch onto baseline. All-or-nothing: any conflict (see /// `resp.conflicts`) or post-merge validation Error (see `resp.validation`) /// persists nothing. `operation_id` is dropped when `options.dry_run` is @@ -334,6 +395,47 @@ mod tests { } } + fn issue(severity: pb::validation_issue::Severity) -> pb::ValidationIssue { + pb::ValidationIssue { + severity: severity as i32, + ..Default::default() + } + } + + #[test] + fn severity_counts_group_findings_and_ignore_unspecified() { + use pb::validation_issue::Severity; + let counts = SeverityCounts::of(&[ + issue(Severity::Warning), + issue(Severity::Info), + issue(Severity::Warning), + issue(Severity::Unspecified), + ]); + assert_eq!( + counts, + SeverityCounts { + errors: 0, + warnings: 2, + infos: 1 + } + ); + assert_eq!(counts.to_string(), "0 errors, 2 warnings, 1 info"); + } + + #[test] + fn merge_counts_read_the_per_class_fields() { + let resp = pb::MergeBranchResponse { + added_count: 1, + changed_count: 2, + deleted_count: 3, + ..Default::default() + }; + assert_eq!( + MergeCounts::of(&resp).to_string(), + "1 added, 2 changed, 3 deleted" + ); + } + #[test] fn validate_branch_name_rejects_spaces_and_reserved_meta() { assert!(validate_branch_name("ok-branch").is_ok()); diff --git a/rsworkspace/crates/trogon-atlas-mcp/README.md b/rsworkspace/crates/trogon-atlas-mcp/README.md index 738582e1..4c60f8e7 100644 --- a/rsworkspace/crates/trogon-atlas-mcp/README.md +++ b/rsworkspace/crates/trogon-atlas-mcp/README.md @@ -41,6 +41,17 @@ JSON view is easier to consume). The previous behavior, embedding Rust's `Debug` output alongside the binpb, was removed because that format is not wire-stable. +Mutating entity tools (`put_entity`, `put_entity_json`, `batch_mutate`, +`batch_mutate_json`, `delete_entity`, `delete_by_query`, +`retarget_references`, `revert_changeset`, `apply_manifests`) add the scope +the write landed in, because a branch write keeps no durable receipt: + +```json +{ "scope": "branch", "branch": "acme-del", "binpb_base64": "..." } +``` + +`scope` is `baseline` and `branch` is `null` when no branch applies. + ## Error contract Internal helpers (`descriptor_for`, `message_from_json`, diff --git a/rsworkspace/crates/trogon-atlas-mcp/src/server.rs b/rsworkspace/crates/trogon-atlas-mcp/src/server.rs index addee78c..dcdc272a 100644 --- a/rsworkspace/crates/trogon-atlas-mcp/src/server.rs +++ b/rsworkspace/crates/trogon-atlas-mcp/src/server.rs @@ -633,6 +633,49 @@ impl McpServer { self.client.clone() } + /// The scope this server's writes land in, named in every mutating + /// response because a branch write leaves no durable receipt to check. + fn scope_fields(&self) -> (&'static str, Option<&str>) { + match self.options.branch.as_deref() { + Some(branch) => ("branch", Some(branch)), + None => ("baseline", None), + } + } + + fn scoped_envelope(&self, msg: &M) -> CallToolResult { + let (scope, branch) = self.scope_fields(); + let body = serde_json::json!({ + "scope": scope, + "branch": branch, + "binpb_base64": encode_b64(msg), + }) + .to_string(); + CallToolResult::success(vec![ContentBlock::text(body)]) + } + + fn scoped_response( + &self, + message_name: &str, + msg: &M, + want_json: bool, + ) -> CallToolResult { + let mut result = ok_response(message_name, msg, want_json); + let (scope, branch) = self.scope_fields(); + let body = result + .content + .first() + .and_then(|block| block.as_text()) + .and_then(|text| serde_json::from_str::(&text.text).ok()); + if let Some(serde_json::Value::Object(mut fields)) = body { + fields.insert("scope".into(), scope.into()); + fields.insert("branch".into(), branch.into()); + result.content = vec![ContentBlock::text( + serde_json::Value::Object(fields).to_string(), + )]; + } + result + } + async fn ensure_can_mutate( &self, client: &mut Client, @@ -1555,7 +1598,7 @@ impl McpServer { .await .map_err(tool_err("retarget_references"))? .into_inner(); - Ok(ok_envelope(&resp)) + Ok(self.scoped_envelope(&resp)) } #[tool( @@ -1747,7 +1790,7 @@ impl McpServer { .map_err(tool_err("put_entity")) .map_err(|e| attach_operation_id(e, &operation_id))? .into_inner(); - Ok(ok_envelope(&resp)) + Ok(self.scoped_envelope(&resp)) } #[tool( @@ -1834,7 +1877,7 @@ impl McpServer { .map_err(tool_err("put_entity_json")) .map_err(|e| attach_operation_id(e, &operation_id))? .into_inner(); - Ok(ok_envelope(&resp)) + Ok(self.scoped_envelope(&resp)) } #[tool( @@ -1879,7 +1922,7 @@ impl McpServer { if resp.status == pb::batch_mutate_response::Status::Failed as i32 { return Err(McpError::from(batch_mutate_failure(&resp)).into()); } - Ok(ok_envelope(&resp)) + Ok(self.scoped_envelope(&resp)) } #[tool( @@ -1914,7 +1957,7 @@ impl McpServer { .map_err(tool_err("delete_entity")) .map_err(|e| attach_operation_id(e, &operation_id))? .into_inner(); - Ok(ok_envelope(&resp)) + Ok(self.scoped_envelope(&resp)) } #[tool( @@ -2042,7 +2085,7 @@ impl McpServer { let err: rmcp::ErrorData = McpError::from(batch_mutate_failure(&resp)).into(); return Err(attach_operation_id(err, &operation_id)); } - Ok(ok_envelope(&resp)) + Ok(self.scoped_envelope(&resp)) } #[tool( @@ -2185,7 +2228,7 @@ impl McpServer { .map_err(tool_err("revert_changeset")) .map_err(|e| attach_operation_id(e, &operation_id))? .into_inner(); - Ok(ok_envelope(&resp)) + Ok(self.scoped_envelope(&resp)) } #[tool( @@ -2245,7 +2288,7 @@ impl McpServer { } #[tool( - description = "Validate every EventModel in a project (or domain) in one call instead of N. Returns per-model issue lists plus aggregate Error/Warning/Info counts. Scope by project_namespace+project_slug OR by domain_namespace+domain_slug -- set exactly one.", + description = "Validate every EventModel in a project, domain or namespace in one call instead of N. Returns per-model issue lists plus aggregate Error/Warning/Info counts. Scope by project_namespace+project_slug, by domain_namespace+domain_slug, OR by namespace -- set exactly one.", annotations( read_only_hint = true, destructive_hint = false, @@ -2259,13 +2302,27 @@ impl McpServer { ) -> Result { let has_project = p.project_namespace.is_some() || p.project_slug.is_some(); let has_domain = p.domain_namespace.is_some() || p.domain_slug.is_some(); - if has_project && has_domain { + let has_namespace = p.namespace.is_some(); + if [has_project, has_domain, has_namespace] + .into_iter() + .filter(|set| *set) + .count() + > 1 + { return Err(rmcp::ErrorData::invalid_params( - "scope is exclusive: set project_namespace+project_slug OR domain_namespace+domain_slug, not both", + "scope is exclusive: set project_namespace+project_slug, domain_namespace+domain_slug OR namespace, not several", None, )); } - let scope = if let (Some(ns), Some(slug)) = + let scope = if let Some(ns) = p.namespace.as_ref() { + if ns.is_empty() { + return Err(rmcp::ErrorData::invalid_params( + "namespace must not be empty", + None, + )); + } + pb::validate_project_request::Scope::Namespace(ns.clone()) + } else if let (Some(ns), Some(slug)) = (p.project_namespace.as_ref(), p.project_slug.as_ref()) { let version = p.project_version.ok_or_else(|| { @@ -2298,7 +2355,7 @@ impl McpServer { pb::validate_project_request::Scope::DomainId(validated_id_from(ns, slug, version)?) } else { return Err(rmcp::ErrorData::invalid_params( - "scope is required: set project_namespace+project_slug OR domain_namespace+domain_slug", + "scope is required: set project_namespace+project_slug, domain_namespace+domain_slug OR namespace", None, )); }; @@ -2361,7 +2418,7 @@ impl McpServer { .await .map_err(tool_err("delete_by_query"))? .into_inner(); - Ok(ok_response( + Ok(self.scoped_response( "trogonatlas.api.eventmodel.v1alpha1.DeleteByQueryResponse", &resp, p.json.unwrap_or(false), @@ -2530,7 +2587,10 @@ impl McpServer { .map_err(|e| { attach_operation_id(rmcp::ErrorData::from(McpError::from(e)), &operation_id) })?; + let (scope, branch) = self.scope_fields(); let body = serde_json::json!({ + "scope": scope, + "branch": branch, "lines": outcome.lines, "issues": outcome.issues.iter().map(trogon_atlas_client::apply::format_issue).collect::>(), "operation_id": (!operation_id.is_empty()).then_some(operation_id.as_str()), @@ -2725,7 +2785,7 @@ impl McpServer { } #[tool( - description = "Merge a branch onto baseline (Phase 2: Review and merge). All-or-nothing: any conflict (see conflicts) or post-merge validation Error (see validation) persists nothing. Set dry_run to run full detection and validation without persisting even on success. keep_branch controls whether a successful, non-dry-run merge deletes the branch afterward (default: deletes it). Set auto_merge to land the combined result for edit/edit conflicts whose two sides changed different fields.", + description = "Merge a branch onto baseline (Phase 2: Review and merge). All-or-nothing: any conflict (see conflicts) or post-merge validation Error (see validation) persists nothing. Set dry_run to run full detection and validation without persisting even on success; the result splits the landable entries into added_count, changed_count and deleted_count and lists the post-merge findings in validation. keep_branch controls whether a successful, non-dry-run merge deletes the branch afterward (default: deletes it). Set auto_merge to land the combined result for edit/edit conflicts whose two sides changed different fields.", annotations( read_only_hint = false, destructive_hint = true, diff --git a/rsworkspace/crates/trogon-atlas-mcp/src/tools.rs b/rsworkspace/crates/trogon-atlas-mcp/src/tools.rs index 62d4bb9f..86e5fe9e 100644 --- a/rsworkspace/crates/trogon-atlas-mcp/src/tools.rs +++ b/rsworkspace/crates/trogon-atlas-mcp/src/tools.rs @@ -327,6 +327,8 @@ struct ValidateProjectParamsDe { #[serde(default)] domain_version: Option, #[serde(default)] + namespace: Option, + #[serde(default)] json: Option, } @@ -345,6 +347,9 @@ pub struct ValidateProjectParams { pub domain_slug: Option, /// Required (>0) when domain_namespace+domain_slug are set. Omitted must not become 0. pub domain_version: Option, + /// Validate every EventModel in this namespace. Exclusive with the project and domain scopes. + #[serde(default)] + pub namespace: Option, #[serde(default)] pub json: Option, } @@ -362,9 +367,15 @@ impl TryFrom for ValidateProjectParams { fn try_from(raw: ValidateProjectParamsDe) -> Result { let has_project = raw.project_namespace.is_some() || raw.project_slug.is_some(); let has_domain = raw.domain_namespace.is_some() || raw.domain_slug.is_some(); - if has_project && has_domain { + let has_namespace = raw.namespace.is_some(); + if [has_project, has_domain, has_namespace] + .into_iter() + .filter(|set| *set) + .count() + > 1 + { return Err( - "scope is exclusive: set project_namespace+project_slug OR domain_namespace+domain_slug, not both", + "scope is exclusive: set project_namespace+project_slug, domain_namespace+domain_slug OR namespace, not several", ); } if has_project { @@ -398,6 +409,7 @@ impl TryFrom for ValidateProjectParams { domain_namespace: raw.domain_namespace, domain_slug: raw.domain_slug, domain_version: raw.domain_version, + namespace: raw.namespace, json: raw.json, }) } @@ -1113,6 +1125,26 @@ mod deny_unknown_params_tests { ); } + #[test] + fn validate_project_params_accept_a_namespace_scope() { + let parsed = serde_json::from_value::(serde_json::json!({ + "namespace": "acme" + })) + .unwrap(); + assert_eq!(parsed.namespace.as_deref(), Some("acme")); + } + + #[test] + fn validate_project_params_reject_a_namespace_beside_another_scope() { + let parsed = serde_json::from_value::(serde_json::json!({ + "namespace": "acme", + "domain_namespace": "acme", + "domain_slug": "commerce", + "domain_version": 1 + })); + assert!(parsed.is_err(), "got {parsed:?}"); + } + // Server DeleteByQuery rejects max_deletes=0 (FailedPrecondition); the MCP // schema must not document "0 = unlimited" or let omitted become 0. #[test] diff --git a/rsworkspace/crates/trogon-atlas-mcp/tests/mutation_scope_mcp_integration.rs b/rsworkspace/crates/trogon-atlas-mcp/tests/mutation_scope_mcp_integration.rs new file mode 100644 index 00000000..c7a5d5bd --- /dev/null +++ b/rsworkspace/crates/trogon-atlas-mcp/tests/mutation_scope_mcp_integration.rs @@ -0,0 +1,128 @@ +#![allow(clippy::unwrap_used, clippy::expect_used)] + +mod support; + +// Every mutating tool names the scope it wrote to, so a caller can tell a +// branch write from a baseline write without exporting and diffing both. + +use std::sync::Arc; + +use tokio::net::TcpListener; +use trogon_atlas_proto::event_model_service_server::EventModelServiceServer; +use trogon_atlas_server::{metrics::ServerMetrics, service::EventModelServiceImpl}; + +async fn start_server() -> String { + let listener = TcpListener::bind("127.0.0.1:0").await.unwrap(); + let addr = listener.local_addr().unwrap(); + let store = trogon_atlas_testsupport::shared().await.store().await; + let svc = EventModelServiceImpl::try_new(Arc::new(store), ServerMetrics::default()).unwrap(); + let grpc = EventModelServiceServer::new(svc); + tokio::spawn(async move { + tonic::transport::Server::builder() + .add_service(grpc) + .serve_with_incoming(tokio_stream::wrappers::TcpListenerStream::new(listener)) + .await + .unwrap(); + }); + format!("http://{addr}") +} + +type McpClient = rmcp::service::RunningService; +type McpServiceRunning = + rmcp::service::RunningService; + +async fn mcp_client_for( + grpc_endpoint: &str, + branch: Option<&str>, +) -> (McpClient, McpServiceRunning) { + let mcp_server = support::mcp_server(grpc_endpoint, branch.map(str::to_string)); + let (client_half, server_half) = tokio::io::duplex(64 * 1024); + use rmcp::ServiceExt; + let (mcp_service_result, client_result) = tokio::join!( + mcp_server.serve(server_half), + rmcp::serve_client((), client_half) + ); + (client_result.unwrap(), mcp_service_result.unwrap()) +} + +fn args_obj(pairs: &[(&str, serde_json::Value)]) -> serde_json::Map { + let mut m = serde_json::Map::new(); + for (k, v) in pairs { + m.insert((*k).to_string(), v.clone()); + } + m +} + +fn body_of(result: &rmcp::model::CallToolResult) -> serde_json::Value { + assert!(!result.is_error.unwrap_or(false), "{result:?}"); + serde_json::from_str(&result.content[0].as_text().unwrap().text).unwrap() +} + +fn event_json(ns: &str, slug: &str) -> String { + serde_json::json!({ + "event": { + "id": {"namespace": ns, "slug": slug, "version": 1}, + "title": slug, + } + }) + .to_string() +} + +async fn put_then_delete(client: &McpClient, ns: &str) -> (serde_json::Value, serde_json::Value) { + let put = client + .call_tool( + rmcp::model::CallToolRequestParams::new("put_entity_json").with_arguments(args_obj(&[ + ("entity_json", serde_json::json!(event_json(ns, "scoped"))), + ])), + ) + .await + .unwrap(); + let delete = client + .call_tool( + rmcp::model::CallToolRequestParams::new("delete_entity").with_arguments(args_obj(&[ + ("kind", serde_json::json!("Event")), + ("namespace", serde_json::json!(ns)), + ("slug", serde_json::json!("scoped")), + ("version", serde_json::json!(1)), + ])), + ) + .await + .unwrap(); + (body_of(&put), body_of(&delete)) +} + +#[tokio::test(flavor = "multi_thread")] +async fn branch_scoped_mutations_name_their_branch() { + let grpc_endpoint = start_server().await; + let (admin, admin_service) = mcp_client_for(&grpc_endpoint, None).await; + admin + .call_tool( + rmcp::model::CallToolRequestParams::new("create_branch").with_arguments(args_obj(&[ + ("name", serde_json::json!("scope-echo")), + ("doc", serde_json::json!("")), + ])), + ) + .await + .unwrap(); + + let (client, service) = mcp_client_for(&grpc_endpoint, Some("scope-echo")).await; + let (put, delete) = put_then_delete(&client, "scope-echo-ns").await; + for body in [&put, &delete] { + assert_eq!(body["scope"], "branch", "{body}"); + assert_eq!(body["branch"], "scope-echo", "{body}"); + } + service.cancel().await.unwrap(); + admin_service.cancel().await.unwrap(); +} + +#[tokio::test(flavor = "multi_thread")] +async fn baseline_mutations_say_baseline() { + let grpc_endpoint = start_server().await; + let (client, service) = mcp_client_for(&grpc_endpoint, None).await; + let (put, delete) = put_then_delete(&client, "scope-baseline-ns").await; + for body in [&put, &delete] { + assert_eq!(body["scope"], "baseline", "{body}"); + assert!(body["branch"].is_null(), "{body}"); + } + service.cancel().await.unwrap(); +} diff --git a/rsworkspace/crates/trogon-atlas-mcp/tests/tool_surface_gap_coverage.rs b/rsworkspace/crates/trogon-atlas-mcp/tests/tool_surface_gap_coverage.rs index 4906aece..d3847863 100644 --- a/rsworkspace/crates/trogon-atlas-mcp/tests/tool_surface_gap_coverage.rs +++ b/rsworkspace/crates/trogon-atlas-mcp/tests/tool_surface_gap_coverage.rs @@ -802,6 +802,51 @@ async fn mcp_batch_mutate_binpb_variant() { mcp_service.cancel().await.unwrap(); } +// validate_project: a namespace alone scopes the call to every EventModel in it. +#[tokio::test(flavor = "multi_thread")] +async fn mcp_validate_project_accepts_a_namespace_scope() { + let (client, _mcp_service, grpc_endpoint) = start_mcp_client().await; + let mut grpc = trogon_atlas_client::client::connect(&grpc_endpoint, None, 30) + .await + .unwrap(); + + for (ns, slug) in [("vpns-a", "checkout"), ("vpns-b", "billing")] { + grpc.put_entity(put_req( + pb::Entity { + system: None, + kind: Some(pb::entity::Kind::EventModel(pb::EventModel { + id: Some(pb::Id { + namespace: ns.into(), + slug: slug.into(), + version: 1, + }), + title: slug.into(), + ..Default::default() + })), + }, + true, + )) + .await + .unwrap(); + } + + let result = client + .call_tool( + rmcp::model::CallToolRequestParams::new("validate_project").with_arguments(args_obj( + &[ + ("namespace", serde_json::json!("vpns-a")), + ("json", serde_json::json!(true)), + ], + )), + ) + .await + .unwrap(); + assert!(!result.is_error.unwrap_or(false), "{result:?}"); + let text = format!("{result:?}"); + assert!(text.contains("checkout"), "{text}"); + assert!(!text.contains("billing"), "{text}"); +} + // validate_project: scope by project_namespace+project_slug; scope by // domain_namespace+domain_slug; and the invalid_params rejection when // neither scope is set. diff --git a/rsworkspace/crates/trogon-atlas-proto/src/gen/descriptor.binpb b/rsworkspace/crates/trogon-atlas-proto/src/gen/descriptor.binpb index 19fad536..35e7f1f8 100644 Binary files a/rsworkspace/crates/trogon-atlas-proto/src/gen/descriptor.binpb and b/rsworkspace/crates/trogon-atlas-proto/src/gen/descriptor.binpb differ diff --git a/rsworkspace/crates/trogon-atlas-proto/src/gen/trogonatlas.api.eventmodel.v1alpha1.rs b/rsworkspace/crates/trogon-atlas-proto/src/gen/trogonatlas.api.eventmodel.v1alpha1.rs index f52fce76..0aa1a5c9 100644 --- a/rsworkspace/crates/trogon-atlas-proto/src/gen/trogonatlas.api.eventmodel.v1alpha1.rs +++ b/rsworkspace/crates/trogon-atlas-proto/src/gen/trogonatlas.api.eventmodel.v1alpha1.rs @@ -650,31 +650,31 @@ pub struct ListValidationRulesResponse { #[prost(message, repeated, tag="1")] pub rules: ::prost::alloc::vec::Vec, } -/// Validate every EventModel in a project (or in the entire store) in one +/// Validate every EventModel in a project, a domain or a namespace in one /// pass, instead of N round trips through ValidateEventModel. The /// validator already has the full entity store in memory per call; /// looping per-model is pure waste. #[derive(Clone, PartialEq, Eq, Hash, ::prost::Message)] pub struct ValidateProjectRequest { - /// Scope by Project.id (if your store carries Projects) or by - /// Domain.id (legacy). One of the two must be set. ENTITY_KIND_UNSPECIFIED - /// for the full store works only on small dev stores; large stores - /// should always scope. - #[prost(oneof="validate_project_request::Scope", tags="1, 2")] + /// Scope by Project.id (if your store carries Projects), by Domain.id + /// (legacy) or by namespace. Exactly one must be set. + #[prost(oneof="validate_project_request::Scope", tags="1, 2, 3")] pub scope: ::core::option::Option, } /// Nested message and enum types in `ValidateProjectRequest`. pub mod validate_project_request { - /// Scope by Project.id (if your store carries Projects) or by - /// Domain.id (legacy). One of the two must be set. ENTITY_KIND_UNSPECIFIED - /// for the full store works only on small dev stores; large stores - /// should always scope. + /// Scope by Project.id (if your store carries Projects), by Domain.id + /// (legacy) or by namespace. Exactly one must be set. #[derive(Clone, PartialEq, Eq, Hash, ::prost::Oneof)] pub enum Scope { #[prost(message, tag="1")] ProjectId(super::super::super::super::r#type::v1alpha1::Id), #[prost(message, tag="2")] DomainId(super::super::super::super::r#type::v1alpha1::Id), + /// Every EventModel whose id carries this namespace, whether or not a + /// Domain claims it. + #[prost(string, tag="3")] + Namespace(::prost::alloc::string::String), } } #[derive(Clone, PartialEq, ::prost::Message)] @@ -2131,14 +2131,29 @@ pub struct MergeBranchResponse { pub status: i32, #[prost(message, repeated, tag="2")] pub conflicts: ::prost::alloc::vec::Vec, + /// Post-merge validation findings. STATUS_INVALID carries the findings that + /// blocked the merge. A dry run that reports STATUS_VALIDATED also carries + /// its Warning and Info findings, so a reviewer sees what the merge leaves + /// behind without a separate validation call. #[prost(message, repeated, tag="3")] pub validation: ::prost::alloc::vec::Vec, - /// Number of non-CONVERGED entries actually landed on baseline. + /// Number of non-CONVERGED entries actually landed on baseline, or that a + /// dry run would land. #[prost(uint32, tag="4")] pub applied_count: u32, /// Set only when the request carried an operation_id. #[prost(message, optional, tag="5")] pub operation_receipt: ::core::option::Option, + /// The landed (or, on a dry run, landable) entries split by diff class. + /// Together they sum to applied_count when the merge reached the landing + /// step (STATUS_APPLIED, STATUS_VALIDATED, or a dry run's STATUS_INVALID); + /// all zero for STATUS_CONFLICTS and for an operation_id replay. + #[prost(uint32, tag="6")] + pub added_count: u32, + #[prost(uint32, tag="7")] + pub changed_count: u32, + #[prost(uint32, tag="8")] + pub deleted_count: u32, } /// Nested message and enum types in `MergeBranchResponse`. pub mod merge_branch_response { diff --git a/rsworkspace/crates/trogon-atlas-server/src/service.rs b/rsworkspace/crates/trogon-atlas-server/src/service.rs index 0c266cc0..a8ae644b 100644 --- a/rsworkspace/crates/trogon-atlas-server/src/service.rs +++ b/rsworkspace/crates/trogon-atlas-server/src/service.rs @@ -5979,8 +5979,11 @@ impl pb::event_model_service_server::EventModelService for EventModelServiceImpl let branch = Self::branch_from_metadata(&req)?; let req = req.into_inner(); let scope = req.scope.ok_or_else(|| { - Status::invalid_argument("scope is required (project_id or domain_id)") + Status::invalid_argument("scope is required (project_id, domain_id or namespace)") })?; + if matches!(&scope, Scope::Namespace(ns) if ns.is_empty()) { + return Err(Status::invalid_argument("namespace must not be empty")); + } let all = self.snapshot_for(branch.as_deref(), &vis).await?; @@ -5997,6 +6000,7 @@ impl pb::event_model_service_server::EventModelService for EventModelServiceImpl s.insert((id.namespace.clone(), id.slug.clone())); s } + Scope::Namespace(_) => std::collections::HashSet::new(), Scope::ProjectId(pid) => all .iter() .filter_map(|se| match se.entity.kind.as_ref()? { @@ -6019,10 +6023,11 @@ impl pb::event_model_service_server::EventModelService for EventModelServiceImpl continue; }; let Some(id) = em.id.as_ref() else { continue }; - let owning = bc_domain.get(&id.namespace); - let in_scope = match owning { - Some(did) => scoped_domains.contains(did), - None => false, + let in_scope = match &scope { + Scope::Namespace(ns) => id.namespace == *ns, + _ => bc_domain + .get(&id.namespace) + .is_some_and(|did| scoped_domains.contains(did)), }; if !in_scope { continue; @@ -6659,9 +6664,7 @@ impl pb::event_model_service_server::EventModelService for EventModelServiceImpl return Ok(Response::new(pb::MergeBranchResponse { status: pb::merge_branch_response::Status::Conflicts as i32, conflicts, - validation: Vec::new(), - applied_count: 0, - operation_receipt: None, + ..Default::default() })); } @@ -6679,6 +6682,7 @@ impl pb::event_model_service_server::EventModelService for EventModelServiceImpl ) }) .collect(); + let landed = LandedCounts::of(landable.iter().map(|e| e.status)); let mut ops: Vec = Vec::with_capacity(landable.len()); @@ -6838,18 +6842,14 @@ impl pb::event_model_service_server::EventModelService for EventModelServiceImpl if has_error { return Ok(Response::new(pb::MergeBranchResponse { status: pb::merge_branch_response::Status::Invalid as i32, - conflicts: Vec::new(), validation: validation_issues, - applied_count: 0, - operation_receipt: None, + ..landed.into_response() })); } return Ok(Response::new(pb::MergeBranchResponse { status: pb::merge_branch_response::Status::Validated as i32, - conflicts: Vec::new(), - validation: Vec::new(), - applied_count: u32::try_from(landable.len()).unwrap_or(u32::MAX), - operation_receipt: None, + validation: validation_issues, + ..landed.into_response() })); } @@ -6905,6 +6905,7 @@ impl pb::event_model_service_server::EventModelService for EventModelServiceImpl validation: validation_issues, applied_count: 0, operation_receipt: None, + ..landed.into_response() })); } } @@ -6953,13 +6954,11 @@ impl pb::event_model_service_server::EventModelService for EventModelServiceImpl // caller's proof the merge is settled. return Ok(Response::new(pb::MergeBranchResponse { status: pb::merge_branch_response::Status::Applied as i32, - conflicts: Vec::new(), - validation: Vec::new(), - applied_count: 0, operation_receipt: Some(operation_receipts::replay_receipt( &req.operation_id, &record, )), + ..Default::default() })); } } @@ -6983,12 +6982,10 @@ impl pb::event_model_service_server::EventModelService for EventModelServiceImpl } return Ok(Response::new(pb::MergeBranchResponse { status: pb::merge_branch_response::Status::Applied as i32, - conflicts: Vec::new(), - validation: Vec::new(), - applied_count: 0, operation_receipt: claim_key .as_ref() .map(|_| operation_receipts::fresh_receipt(&req.operation_id, "")), + ..Default::default() })); } @@ -7047,12 +7044,10 @@ impl pb::event_model_service_server::EventModelService for EventModelServiceImpl Ok(Response::new(pb::MergeBranchResponse { status: pb::merge_branch_response::Status::Applied as i32, - conflicts: Vec::new(), - validation: Vec::new(), - applied_count: u32::try_from(ops.len()).unwrap_or(u32::MAX), operation_receipt: claim_key .as_ref() .map(|_| operation_receipts::fresh_receipt(&req.operation_id, &merge_changeset_id)), + ..landed.into_response() })) } @@ -7520,6 +7515,43 @@ fn branch_diff_entry_status(raw: i32) -> pb::branch_diff_entry::Status { .unwrap_or(pb::branch_diff_entry::Status::Unspecified) } +/// How many entries a merge lands, split by diff class. +#[derive(Default)] +struct LandedCounts { + added: u32, + changed: u32, + deleted: u32, +} + +impl LandedCounts { + fn of(statuses: impl IntoIterator) -> Self { + let mut counts = Self::default(); + for status in statuses { + let slot = match branch_diff_entry_status(status) { + pb::branch_diff_entry::Status::Added => &mut counts.added, + pb::branch_diff_entry::Status::Changed => &mut counts.changed, + pb::branch_diff_entry::Status::Deleted => &mut counts.deleted, + _ => continue, + }; + *slot = slot.saturating_add(1); + } + counts + } + + fn into_response(self) -> pb::MergeBranchResponse { + pb::MergeBranchResponse { + applied_count: self + .added + .saturating_add(self.changed) + .saturating_add(self.deleted), + added_count: self.added, + changed_count: self.changed, + deleted_count: self.deleted, + ..Default::default() + } + } +} + /// True for any of the four conflict classes (see /// `docs/explanation/branching.md`, "Merge conflicts"). CONVERGED is /// deliberately excluded: it is not a conflict, it is a no-op entry that is diff --git a/rsworkspace/crates/trogon-atlas-server/src/validation.rs b/rsworkspace/crates/trogon-atlas-server/src/validation.rs index 2ebd969a..e0af7c17 100644 --- a/rsworkspace/crates/trogon-atlas-server/src/validation.rs +++ b/rsworkspace/crates/trogon-atlas-server/src/validation.rs @@ -523,6 +523,9 @@ pub fn rule_catalog() -> Vec { r("SCENARIO_MISSING_REF", Error, "Scenario step missing entity reference", "Each Given/When/Then step must point at a concrete entity (event / command / read model).", Unspecified, "", "scenarios"), + r("SLICE_NO_SCENARIOS", Info, "Slice has no given/when/then scenario", + "A command, read model or automation slice the model includes declares no scenarios, so nothing specifies its behavior. Informational: filter validate_project findings on this code to list the slices still to specify.", + Unspecified, "scenarios", "scenarios"), r("SCENARIO_DANGLING_REF", Error, "Scenario step references missing entity", "A Given/When/Then step points at an entity that is not in the store.", Unspecified, "", "scenarios"), @@ -632,6 +635,7 @@ mod catalog_drift_tests { "SCREEN_NAMESPACE_IS_CONTEXT", "SEAM_WITHOUT_RELATIONSHIP", "SLICE_NOT_IN_STORYBOARD", + "SLICE_NO_SCENARIOS", "SLICE_STALE_REF", "SLI_CONNECTION_UNRESOLVED", "SLI_CORRELATION_UNSOURCED", @@ -4795,6 +4799,57 @@ pub fn validate_model(model: &pb::EventModel, all: &[StoredEntity]) -> Vec s.scenarios.len(), + Some(pb::entity::Kind::ReadModelSlice(s)) => s.scenarios.len(), + Some(pb::entity::Kind::AutomationSlice(s)) => s.scenarios.len(), + _ => continue, + }; + if scenario_count == 0 { + issues.push(pb::ValidationIssue { + severity: pb::validation_issue::Severity::Info as i32, + code: "SLICE_NO_SCENARIOS".into(), + message: format!( + "{}:{}/{}@{} has no given/when/then scenarios", + pb::canonical::kind_short(k), + id.namespace, + id.slug, + id.version, + ), + subject: Some(pb::EntityRef { + kind: m.kind, + id: Some(id.clone()), + }), + subject_field: "scenarios".into(), + ..Default::default() + }); + } + } + // A storyboard walking a slice the EventModel has not claimed is a // curation gap: the storyboard knows the moment, the model does // not. Mirror of SLICE_NOT_IN_STORYBOARD. @@ -9047,6 +9102,62 @@ mod slice_stale_ref_tests { } } + #[test] + fn slice_without_scenarios_is_reported_as_info() { + let all = vec![ + stored(ui_v("composer", 1)), + stored(command_v("post", 1)), + stored(event_v("posted", 1)), + stored(command_slice_pinning( + "post-slice", + id_v("composer", 1), + id_v("post", 1), + vec![id_v("posted", 1)], + )), + ]; + let issues = validate_model(&model_with_slice("post-slice"), &all); + let found: Vec<_> = issues + .iter() + .filter(|i| i.code == "SLICE_NO_SCENARIOS") + .collect(); + assert_eq!(found.len(), 1, "{issues:?}"); + assert_eq!( + found[0].severity, + pb::validation_issue::Severity::Info as i32 + ); + assert_eq!( + found[0].subject.as_ref().and_then(|s| s.id.as_ref()), + Some(&id_v("post-slice", 1)) + ); + } + + #[test] + fn slice_with_a_scenario_is_not_reported() { + let mut slice = command_slice_pinning( + "post-slice", + id_v("composer", 1), + id_v("post", 1), + vec![id_v("posted", 1)], + ); + if let Some(pb::entity::Kind::CommandSlice(s)) = slice.kind.as_mut() { + s.scenarios.push(pb::CommandScenario { + title: "posts".into(), + ..Default::default() + }); + } + let all = vec![ + stored(ui_v("composer", 1)), + stored(command_v("post", 1)), + stored(event_v("posted", 1)), + stored(slice), + ]; + let issues = validate_model(&model_with_slice("post-slice"), &all); + assert!( + !issues.iter().any(|i| i.code == "SLICE_NO_SCENARIOS"), + "{issues:?}", + ); + } + #[test] fn ref_pinning_latest_is_clean() { let all = vec![ diff --git a/rsworkspace/crates/trogon-atlas-server/tests/branching.rs b/rsworkspace/crates/trogon-atlas-server/tests/branching.rs index 1688f60a..dc5c0f35 100644 --- a/rsworkspace/crates/trogon-atlas-server/tests/branching.rs +++ b/rsworkspace/crates/trogon-atlas-server/tests/branching.rs @@ -1702,6 +1702,115 @@ async fn merge_branch_dry_run_validates_without_persisting() { assert_eq!(branch.delta_count, 1); } +/// A reviewer reads one dry-run result instead of running `DiffBranch` and a +/// validation call beside it: the landable entries arrive split by class, +/// and the post-merge findings arrive even when none of them blocks. +#[tokio::test(flavor = "multi_thread")] +async fn merge_branch_dry_run_splits_counts_by_class_and_reports_findings() { + let endpoint = start_server().await; + let mut c = client(&endpoint).await; + + c.put_entity(put_req(event("p2split", "to-change", 1, "Before"), false)) + .await + .unwrap(); + c.put_entity(put_req(event("p2split", "to-delete", 1, "Doomed"), false)) + .await + .unwrap(); + c.create_branch(pb::CreateBranchRequest { + name: "split-branch".into(), + doc: String::new(), + }) + .await + .unwrap(); + c.put_entity(with_branch( + put_req( + pb::Entity { + system: None, + kind: Some(pb::entity::Kind::Domain(pb::Domain { + id: Some(id("p2split", "to-add", 1)), + title: "Fresh".into(), + ..Default::default() + })), + }, + false, + ), + "split-branch", + )) + .await + .unwrap(); + c.put_entity(with_branch( + put_req(event("p2split", "to-change", 1, "After"), false), + "split-branch", + )) + .await + .unwrap(); + c.delete_entity(with_branch( + pb::DeleteEntityRequest { + operation_id: String::new(), + kind: pb::EntityKind::Event as i32, + id: Some(id("p2split", "to-delete", 1)), + if_match: String::new(), + mode: pb::delete_entity_request::Mode::Force as i32, + }, + "split-branch", + )) + .await + .unwrap(); + + let resp = c + .merge_branch(pb::MergeBranchRequest { + operation_id: String::new(), + name: "split-branch".into(), + dry_run: true, + keep_branch: false, + auto_merge: false, + }) + .await + .unwrap() + .into_inner(); + assert_eq!( + resp.status, + pb::merge_branch_response::Status::Validated as i32 + ); + assert_eq!( + ( + resp.added_count, + resp.changed_count, + resp.deleted_count, + resp.applied_count + ), + (1, 1, 1, 3) + ); + assert!( + !resp.validation.is_empty(), + "a dry run must report the findings the merge leaves behind" + ); + assert!(resp + .validation + .iter() + .all(|i| i.severity != pb::validation_issue::Severity::Error as i32)); + + let applied = c + .merge_branch(pb::MergeBranchRequest { + operation_id: String::new(), + name: "split-branch".into(), + dry_run: false, + keep_branch: false, + auto_merge: false, + }) + .await + .unwrap() + .into_inner(); + assert_eq!( + ( + applied.added_count, + applied.changed_count, + applied.deleted_count + ), + (1, 1, 1) + ); +} + /// The dry-run overlay path (`req.dry_run` branch inside `merge_branch`) /// must build a `MutationOp::Delete` for landable entries whose diff status /// is `Deleted`, not just `MutationOp::Put` for adds/changes: validate a @@ -1836,6 +1945,11 @@ async fn merge_branch_invalid_via_dangling_reference() { .iter() .any(|i| i.code == "TERM_DANGLING_ENTITY" && i.severity == pb::validation_issue::Severity::Error as i32)); + assert_eq!(resp.applied_count, 0); + assert_eq!( + (resp.added_count, resp.changed_count, resp.deleted_count), + (1, 0, 0) + ); // Nothing persisted. let err = c diff --git a/rsworkspace/crates/trogon-atlas-server/tests/handler_tests.rs b/rsworkspace/crates/trogon-atlas-server/tests/handler_tests.rs index 8e50a88b..b9865592 100644 --- a/rsworkspace/crates/trogon-atlas-server/tests/handler_tests.rs +++ b/rsworkspace/crates/trogon-atlas-server/tests/handler_tests.rs @@ -77,6 +77,68 @@ async fn validate_project_domain_scope_returns_empty_on_empty_store() { assert_eq!(resp.total_errors, 0); } +fn make_event_model(ns: &str, slug: &str) -> pb::Entity { + pb::Entity { + system: None, + kind: Some(pb::entity::Kind::EventModel(pb::EventModel { + id: Some(id(ns, slug, 1)), + title: slug.into(), + ..Default::default() + })), + } +} + +#[tokio::test(flavor = "multi_thread")] +async fn validate_project_namespace_scope_covers_only_that_namespace() { + let svc = svc().await; + for (ns, slug) in [ + ("vp-ns-a", "checkout"), + ("vp-ns-a", "refunds"), + ("vp-ns-b", "billing"), + ] { + svc.put_entity(Request::new(pb::PutEntityRequest { + operation_id: String::new(), + entity: Some(make_event_model(ns, slug)), + create_only: true, + if_match: String::new(), + validate_only: false, + force: false, + })) + .await + .unwrap(); + } + + let resp = svc + .validate_project(Request::new(pb::ValidateProjectRequest { + scope: Some(pb::validate_project_request::Scope::Namespace( + "vp-ns-a".into(), + )), + })) + .await + .unwrap() + .into_inner(); + let mut validated: Vec = resp + .reports + .iter() + .filter_map(|r| r.event_model_id.as_ref()) + .map(|i| format!("{}/{}", i.namespace, i.slug)) + .collect(); + validated.sort(); + assert_eq!(validated, ["vp-ns-a/checkout", "vp-ns-a/refunds"]); +} + +#[tokio::test(flavor = "multi_thread")] +async fn validate_project_rejects_an_empty_namespace_scope() { + let svc = svc().await; + let err = svc + .validate_project(Request::new(pb::ValidateProjectRequest { + scope: Some(pb::validate_project_request::Scope::Namespace(String::new())), + })) + .await + .unwrap_err(); + assert_eq!(err.code(), tonic::Code::InvalidArgument); +} + // ===== list_entities_by_domain tests ===== #[tokio::test(flavor = "multi_thread")]