diff --git a/crates/config-source/src/lib.rs b/crates/config-source/src/lib.rs index 2d6604d82..8f1239d3d 100644 --- a/crates/config-source/src/lib.rs +++ b/crates/config-source/src/lib.rs @@ -487,7 +487,8 @@ pub struct ConfigSchemaContribution { pub namespace: String, pub version: String, pub source: String, - // Optional editor-only schema; never used to validate or materialize config. + // Optional editor shape for completion and non-materializing constraint analysis; + // never used to materialize or validate the persisted config projection. #[serde(default, skip_serializing_if = "Option::is_none")] pub authoring_source: Option, #[serde(default, skip_serializing_if = "Option::is_none")] @@ -1250,17 +1251,83 @@ impl SnapshotEnvironment { "configured entrypoint is missing", )]; }; - let service = LanguageService::new(self); - service - .analyze( - entrypoint.as_str(), - entrypoint.as_str(), - source_override.unwrap_or(&entry.content), - ) + let source = source_override.unwrap_or(&entry.content); + let diagnostics: Vec<_> = LanguageService::new(self) + .analyze(entrypoint.as_str(), entrypoint.as_str(), source) .diagnostics .iter() .map(|diagnostic| project_diagnostic(&self.snapshot, entrypoint.clone(), diagnostic)) - .collect() + .collect(); + if !diagnostics.is_empty() + || !self.schema_bundle.as_ref().is_some_and(|bundle| { + bundle + .contributions + .iter() + .any(|entry| entry.authoring_source.is_some()) + }) + { + return diagnostics; + } + + // Check authoring constraints separately, without materializing the shape. + // Partial records may omit any recipe field: authoring metadata must neither + // make those fields required nor supply defaults to the saved projection. + let environment = ConfigAuthoringEnvironment { + environment: self, + schema_path: &[], + }; + let mut engine = match environment.create_engine() { + Ok(engine) => engine, + Err(diagnostic) => { + return vec![project_diagnostic( + &self.snapshot, + entrypoint.clone(), + &diagnostic, + )]; + } + }; + let result = engine + .add_root_source(entrypoint.as_str(), entrypoint.as_str(), source) + .and_then(|module| engine.eval_module(module)); + match result { + Ok(_) => Vec::new(), + Err(mut diagnostic) => { + // Constraint failures often point at the schema declaration. Anchor + // the editor diagnostic at the narrowest supplied value instead; + // label spans have no file field, so retain only that source's labels. + if !engine + .source_name(diagnostic.span.source) + .is_some_and(|name| { + VirtualPath::parse(name) + .is_ok_and(|path| self.snapshot.get(&path).is_some()) + }) + && let Some(span) = diagnostic + .labels + .iter() + .map(|label| label.span) + .filter(|span| { + engine.source_name(span.source).is_some_and(|name| { + VirtualPath::parse(name) + .is_ok_and(|path| self.snapshot.get(&path).is_some()) + }) + }) + .min_by_key(|span| span.end.saturating_sub(span.start)) + { + diagnostic.span = span; + } + diagnostic + .labels + .retain(|label| label.span.source == diagnostic.span.source); + let mut projected = project_engine_diagnostic( + &engine, + &self.snapshot, + entrypoint.clone(), + &diagnostic, + ); + projected.kind = diagnostic_kind(diagnostic.kind).to_string(); + vec![projected] + } + } } pub fn complete( @@ -1309,7 +1376,7 @@ impl SnapshotEnvironment { } member_source.push('.'); member_source.push_str(prefix); - let completion_environment = ConfigFieldCompletionEnvironment { + let completion_environment = ConfigAuthoringEnvironment { environment: self, schema_path: if has_array { schema_path } else { &[] }, }; @@ -1379,12 +1446,12 @@ impl HostEnvironment for &SnapshotEnvironment { // Decodal has no indexed member syntax. Resolve array ranges only in this // completion environment, then reuse the language service's member behavior. -struct ConfigFieldCompletionEnvironment<'a> { +struct ConfigAuthoringEnvironment<'a> { environment: &'a SnapshotEnvironment, schema_path: &'a [ConfigCompletionPathSegment], } -impl HostEnvironment for ConfigFieldCompletionEnvironment<'_> { +impl HostEnvironment for ConfigAuthoringEnvironment<'_> { type Loader = SnapshotImportLoader; fn create_loader(&self) -> Self::Loader { @@ -2488,6 +2555,110 @@ mod tests { assert_eq!(environment.evaluate_contract(&contract).unwrap(), evaluated); } + #[test] + fn actual_profile_schema_diagnoses_nested_values_without_materializing_omissions() { + let schema = WorkspaceConfigSchemaBundle::compose([ConfigSchemaContribution::new( + "builtin:profile", + "profile", + "2", + include_str!("../../../resources/config-schema/profile.dcdl"), + ) + .unwrap() + .with_authoring_source(include_str!( + "../../../resources/config-schema/profile-authoring.dcdl" + ))]) + .unwrap(); + let source = "{ profile = { entries = [{ selector = \"project:alpha\"; profile = {}; }]; }; } as WorkspaceConfigSchema"; + let snapshot = ConfigTreeSnapshot::from_entries( + 8, + [ + entry("main.dcdl", source), + entry("recipe.dcdl", "{ worker = { mode = 42; }; }"), + ], + ) + .unwrap(); + let environment = SnapshotEnvironment::new(snapshot).with_schema_bundle(schema.clone()); + let contract = ToolchainContract::with_schema_bundle(1, vec![path("main.dcdl")], 1, schema); + let before = environment.evaluate_contract(&contract).unwrap(); + for value in [ + "{}", + "{ worker = {}; }", + "{ worker = { mode = \"wip\"; }; }", + "{ feature = { task = {}; }; }", + ] { + let source = source.replace("profile = {};", &format!("profile = {value};")); + assert!( + environment + .analyze(&path("main.dcdl"), Some(&source)) + .is_empty(), + "{source}" + ); + } + for (value, field, expected_path) in [ + ("{ worker = 42; }", "worker", "main.dcdl"), + ("{ worker = { mode = 42; }; }", "mode", "main.dcdl"), + ( + "{ feature = { task = { enabled = 42; }; }; }", + "enabled", + "main.dcdl", + ), + ("{ worker = { typo = \"wip\"; }; }", "typo", "main.dcdl"), + ("import \"./recipe.dcdl\"", "mode", "recipe.dcdl"), + ( + "(import \"./recipe.dcdl\") // { worker = { mode = 42; }; }", + "mode", + "main.dcdl", + ), + ] { + let source = source.replace("profile = {};", &format!("profile = {value};")); + let diagnostics = environment.analyze(&path("main.dcdl"), Some(&source)); + let diagnostic = diagnostics + .first() + .unwrap_or_else(|| panic!("missing {field} diagnostic for {source}")); + assert!( + matches!( + diagnostic.kind.as_str(), + "constraint_violation" | "type_mismatch" + ), + "{diagnostic:?}" + ); + assert_eq!(diagnostic.path, path(expected_path), "{diagnostic:?}"); + assert_eq!(diagnostic.revision, 8); + let diagnostic_source = if expected_path == "main.dcdl" { + &source + } else { + "{ worker = { mode = 42; }; }" + }; + assert!( + diagnostic.span.end_byte > diagnostic.span.start_byte + && diagnostic.span.end_byte <= diagnostic_source.len() as u32, + "{diagnostic:?}" + ); + assert!( + diagnostic.message.contains(field) + || diagnostic + .labels + .iter() + .any(|label| label.message.contains(field)), + "{diagnostic:?}" + ); + assert!( + diagnostic + .labels + .iter() + .all(|label| label.span.end_byte <= diagnostic_source.len() as u32) + ); + } + // Analysis never changes the evaluation schema or transports authoring fields. + assert_eq!(environment.evaluate_contract(&contract).unwrap(), before); + assert_eq!( + before.projections[0] + .data_json + .pointer("/profile/entries/0/profile"), + Some(&serde_json::json!({})) + ); + } + #[test] fn completion_authoring_sources_compose_with_validation_source_fallbacks() { let schema = WorkspaceConfigSchemaBundle::compose([ diff --git a/docs/design/profiles-manifests-prompts.md b/docs/design/profiles-manifests-prompts.md index 8d5192f39..c75a33fe1 100644 --- a/docs/design/profiles-manifests-prompts.md +++ b/docs/design/profiles-manifests-prompts.md @@ -43,7 +43,7 @@ The imported file can contain `{ worker = { mode = "wip"; }; model = { ref = "co The former `entries = [{ selector = "project:name"; source = "profiles/name.dcdl"; }]` registration is no longer accepted. Migrate it explicitly to `profile = import "./profiles/name.dcdl";`, preserving the selector, label, description and default. For registrations in a nested config file, adjust the relative import to the original virtual-tree entry (for example `../profiles/name.dcdl`). Do not rename or recreate the Profile file merely for registration. Previously saved source-based revisions remain readable as config data and are not rewritten, deleted, or silently substituted with a builtin. Their Profile projection/launch reports `profile_source_registration_removed` with the replacement form; saves under the new schema reject the old field. Existing Workers continue using their saved Manifest while the author migrates and saves the config. -The schema contribution separates its materialized registration schema from an optional, fingerprint-bound `authoring_source`. The latter supplies editor field suggestions inside array elements and partial Profile records; it is never evaluated as defaults or substituted for the validation schema. Decodal structural diagnostics cover registration/value shape, and the shared Backend save boundary checks full Profile/Manifest semantics (including mode, forbidden runtime fields, duplicate selectors and default references). UI and WIP saves use that same boundary; invalid candidates leave the active config revision untouched. +The schema contribution separates its materialized registration schema from an optional, fingerprint-bound `authoring_source`. The latter supplies editor field suggestions inside array elements and partial Profile records, plus a separate constraint-only analysis pass for supplied nested values (including imported recipes); that pass never materializes the shape, makes omitted fields required, or supplies defaults to the saved projection. Editor diagnostics point to the supplied value's virtual file/span. The authoring shape is never substituted for the persisted evaluation schema. Decodal structural diagnostics cover registration and nested Profile field types; the shared Backend save boundary additionally checks full Profile/Manifest semantics (including mode values, forbidden runtime fields, duplicate selectors and default references). UI and WIP saves use that same boundary; invalid candidates leave the active config revision untouched. ## Manifests diff --git a/docs/report/2026-10-06-review-finding-severity.md b/docs/report/2026-10-06-review-finding-severity.md new file mode 100644 index 000000000..72277abf6 --- /dev/null +++ b/docs/report/2026-10-06-review-finding-severity.md @@ -0,0 +1,12 @@ +# Review finding severity の入力契約 + +T-710 の独立レビュー中、Reviewer が `ReviewMergeRequest` に finding severity `medium` を渡し、HTTP 422 で拒否された。Backend の許可値は `blocker` / `major` / `minor` / `note` で、最初の呼び出しでは verdict が記録されなかった。 + +親は committed session と MR thread を確認して未記録であることを確かめ、Reviewer の新しい turn で同じ capability・同じ MR/source に対して `major` を使った訂正を依頼した。訂正は成功し、MR sequence 3 に `request_changes` が記録された。別の保存経路や Ticket comment で verdict を代替しなかった。 + +## 改善案 + +- finding severity は公開 tool schema でも Backend と同じ enum として示し、受理可能な語彙を入力前に確認できるようにする。 +- capability を使う操作の検証エラーでは、イベントが未記録であることと、訂正した入力を再送できるかを明確に返す。Reviewer が「一回の呼び出し」と「一回の有効な verdict 記録」を混同し、未記録のまま止まる障壁を減らせる。 + +これは observed rejection と訂正成功に基づくフィードバックであり、Backend の review authority を迂回する提案ではない。 diff --git a/resources/config-schema/profile-authoring.dcdl b/resources/config-schema/profile-authoring.dcdl index 62317038e..da6fe1853 100644 --- a/resources/config-schema/profile-authoring.dcdl +++ b/resources/config-schema/profile-authoring.dcdl @@ -1,5 +1,6 @@ -# Completion-only shape of partial Profile values. This is never materialized or -# used as defaults: omitted fields remain omitted until Manifest resolution. +# Editor-only shape of partial Profile values, used for completion and constraint +# analysis without materialization. Never used as defaults: omitted fields remain +# omitted until Manifest resolution. let Flag = { enabled = Bool; }; Model = { diff --git a/web/workspace/src/lib/workspace/config-source/generated/config_source_wasm_bg.wasm b/web/workspace/src/lib/workspace/config-source/generated/config_source_wasm_bg.wasm index 8fc0b4245..f6cf6e8cf 100644 Binary files a/web/workspace/src/lib/workspace/config-source/generated/config_source_wasm_bg.wasm and b/web/workspace/src/lib/workspace/config-source/generated/config_source_wasm_bg.wasm differ diff --git a/web/workspace/test/config-source/wasm-parity.test.ts b/web/workspace/test/config-source/wasm-parity.test.ts index 637299db7..cd03c15d5 100644 --- a/web/workspace/test/config-source/wasm-parity.test.ts +++ b/web/workspace/test/config-source/wasm-parity.test.ts @@ -475,19 +475,100 @@ Deno.test("generated WASM authors value-based Profiles using the Backend schema ); assertEquals(result.from, cursor - token.length); } - const invalidContent = content.replace("profile = {};", "profile = 42;"); - const invalid = { + for ( + const value of [ + "{}", + "{ worker = {}; }", + '{ worker = { mode = "wip"; }; }', + "{ feature = { task = {}; }; }", + ] + ) { + const partialContent = content.replace( + "profile = {};", + `profile = ${value};`, + ); + assertEquals( + analyze_snapshot(tree, "main.dcdl", partialContent), + [], + value, + ); + } + const recipe = "{ worker = { mode = 42; }; }"; + const analysisTree = { ...tree, entries: { - "main.dcdl": { ...tree.entries["main.dcdl"], content: invalidContent }, + ...tree.entries, + "recipe.dcdl": { + path: "recipe.dcdl", + content_type: "decodal", + content: recipe, + content_digest: await digestText(recipe), + }, }, }; - const diagnostics = analyze_snapshot( - invalid, - "main.dcdl", - undefined, - ) as Array< - unknown - >; - assertEquals(diagnostics.length > 0, true); + for ( + const [value, field, expectedPath] of [ + ["42", "profile", "main.dcdl"], + ["{ worker = 42; }", "worker", "main.dcdl"], + ["{ worker = { mode = 42; }; }", "mode", "main.dcdl"], + ["{ feature = { task = { enabled = 42; }; }; }", "enabled", "main.dcdl"], + ['{ worker = { typo = "wip"; }; }', "typo", "main.dcdl"], + ['import "./recipe.dcdl"', "mode", "recipe.dcdl"], + [ + '(import "./recipe.dcdl") // { worker = { mode = 42; }; }', + "mode", + "main.dcdl", + ], + ] + ) { + const invalidContent = content.replace( + "profile = {};", + `profile = ${value};`, + ); + const diagnostics = analyze_snapshot( + analysisTree, + "main.dcdl", + invalidContent, + ) as Array<{ + path: string; + revision: number; + kind: string; + span: { start_byte: number; end_byte: number }; + message: string; + labels: Array< + { span: { start_byte: number; end_byte: number }; message: string } + >; + }>; + assertEquals(diagnostics.length > 0, true, value); + const diagnostic = diagnostics[0]; + assertEquals(diagnostic.path, expectedPath); + assertEquals(diagnostic.revision, tree.revision); + assertEquals( + ["constraint_violation", "type_mismatch"].includes(diagnostic.kind), + true, + ); + assertEquals( + diagnostic.message.includes(field) || + diagnostic.labels.some((label) => label.message.includes(field)), + true, + ); + // New authoring failures must be anchored in the supplied value, not the + // much longer schema source, including relative imported recipes. + if (value !== "42") { + const length = new TextEncoder().encode( + expectedPath === "main.dcdl" ? invalidContent : recipe, + ).length; + assertEquals( + diagnostic.span.end_byte > diagnostic.span.start_byte && + diagnostic.span.end_byte <= length, + true, + ); + assertEquals( + diagnostic.labels.every((label) => label.span.end_byte <= length), + true, + ); + } + } + // Neither completion nor diagnostic shape checking changes the saved value. + assertEquals(evaluate_snapshot(tree, valueContract), evaluated); });