fix: diagnose nested partial Profile values in the editor (T-710)
Resolve MR sequence 3 profile_nested_editor_diagnostics_missing with a separate, non-materializing authoring constraint pass. Preserve omission and the persisted evaluation schema, and map nested/imported failures to supplied virtual-source spans. Exercise actual Backend schema in native and regenerated WASM tests for empty/partial values, worker and feature types, unknown fields, imports and composition. Validation: root cargo check; config-source 38; config-source-wasm native/doc; unfiltered yoi-workspace-server 642 lib + 11 bin; worker-runtime 340 lib + 17 per binary; server-api --features typescript 77; focused Deno 39; deno task check; wasm32 release build, wasm-bindgen 0.2.117 + Binaryen 132 -Oz regeneration; cargo fmt/diff checks. Earlier checker failure did not recur: Reviewer independently passed full source suite and exact original baseline checker filter, recorded in MR sequence 3.
This commit is contained in:
+183
-12
@@ -487,7 +487,8 @@ pub struct ConfigSchemaContribution {
|
|||||||
pub namespace: String,
|
pub namespace: String,
|
||||||
pub version: String,
|
pub version: String,
|
||||||
pub source: 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")]
|
#[serde(default, skip_serializing_if = "Option::is_none")]
|
||||||
pub authoring_source: Option<String>,
|
pub authoring_source: Option<String>,
|
||||||
#[serde(default, skip_serializing_if = "Option::is_none")]
|
#[serde(default, skip_serializing_if = "Option::is_none")]
|
||||||
@@ -1250,17 +1251,83 @@ impl SnapshotEnvironment {
|
|||||||
"configured entrypoint is missing",
|
"configured entrypoint is missing",
|
||||||
)];
|
)];
|
||||||
};
|
};
|
||||||
let service = LanguageService::new(self);
|
let source = source_override.unwrap_or(&entry.content);
|
||||||
service
|
let diagnostics: Vec<_> = LanguageService::new(self)
|
||||||
.analyze(
|
.analyze(entrypoint.as_str(), entrypoint.as_str(), source)
|
||||||
entrypoint.as_str(),
|
|
||||||
entrypoint.as_str(),
|
|
||||||
source_override.unwrap_or(&entry.content),
|
|
||||||
)
|
|
||||||
.diagnostics
|
.diagnostics
|
||||||
.iter()
|
.iter()
|
||||||
.map(|diagnostic| project_diagnostic(&self.snapshot, entrypoint.clone(), diagnostic))
|
.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(
|
pub fn complete(
|
||||||
@@ -1309,7 +1376,7 @@ impl SnapshotEnvironment {
|
|||||||
}
|
}
|
||||||
member_source.push('.');
|
member_source.push('.');
|
||||||
member_source.push_str(prefix);
|
member_source.push_str(prefix);
|
||||||
let completion_environment = ConfigFieldCompletionEnvironment {
|
let completion_environment = ConfigAuthoringEnvironment {
|
||||||
environment: self,
|
environment: self,
|
||||||
schema_path: if has_array { schema_path } else { &[] },
|
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
|
// Decodal has no indexed member syntax. Resolve array ranges only in this
|
||||||
// completion environment, then reuse the language service's member behavior.
|
// completion environment, then reuse the language service's member behavior.
|
||||||
struct ConfigFieldCompletionEnvironment<'a> {
|
struct ConfigAuthoringEnvironment<'a> {
|
||||||
environment: &'a SnapshotEnvironment,
|
environment: &'a SnapshotEnvironment,
|
||||||
schema_path: &'a [ConfigCompletionPathSegment],
|
schema_path: &'a [ConfigCompletionPathSegment],
|
||||||
}
|
}
|
||||||
|
|
||||||
impl HostEnvironment for ConfigFieldCompletionEnvironment<'_> {
|
impl HostEnvironment for ConfigAuthoringEnvironment<'_> {
|
||||||
type Loader = SnapshotImportLoader;
|
type Loader = SnapshotImportLoader;
|
||||||
|
|
||||||
fn create_loader(&self) -> Self::Loader {
|
fn create_loader(&self) -> Self::Loader {
|
||||||
@@ -2488,6 +2555,110 @@ mod tests {
|
|||||||
assert_eq!(environment.evaluate_contract(&contract).unwrap(), evaluated);
|
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]
|
#[test]
|
||||||
fn completion_authoring_sources_compose_with_validation_source_fallbacks() {
|
fn completion_authoring_sources_compose_with_validation_source_fallbacks() {
|
||||||
let schema = WorkspaceConfigSchemaBundle::compose([
|
let schema = WorkspaceConfigSchemaBundle::compose([
|
||||||
|
|||||||
@@ -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 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
|
## Manifests
|
||||||
|
|
||||||
|
|||||||
@@ -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 を迂回する提案ではない。
|
||||||
@@ -1,5 +1,6 @@
|
|||||||
# Completion-only shape of partial Profile values. This is never materialized or
|
# Editor-only shape of partial Profile values, used for completion and constraint
|
||||||
# used as defaults: omitted fields remain omitted until Manifest resolution.
|
# analysis without materialization. Never used as defaults: omitted fields remain
|
||||||
|
# omitted until Manifest resolution.
|
||||||
let
|
let
|
||||||
Flag = { enabled = Bool; };
|
Flag = { enabled = Bool; };
|
||||||
Model = {
|
Model = {
|
||||||
|
|||||||
Binary file not shown.
@@ -475,19 +475,100 @@ Deno.test("generated WASM authors value-based Profiles using the Backend schema
|
|||||||
);
|
);
|
||||||
assertEquals(result.from, cursor - token.length);
|
assertEquals(result.from, cursor - token.length);
|
||||||
}
|
}
|
||||||
const invalidContent = content.replace("profile = {};", "profile = 42;");
|
for (
|
||||||
const invalid = {
|
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,
|
...tree,
|
||||||
entries: {
|
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(
|
for (
|
||||||
invalid,
|
const [value, field, expectedPath] of [
|
||||||
"main.dcdl",
|
["42", "profile", "main.dcdl"],
|
||||||
undefined,
|
["{ worker = 42; }", "worker", "main.dcdl"],
|
||||||
) as Array<
|
["{ worker = { mode = 42; }; }", "mode", "main.dcdl"],
|
||||||
unknown
|
["{ feature = { task = { enabled = 42; }; }; }", "enabled", "main.dcdl"],
|
||||||
>;
|
['{ worker = { typo = "wip"; }; }', "typo", "main.dcdl"],
|
||||||
assertEquals(diagnostics.length > 0, true);
|
['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);
|
||||||
});
|
});
|
||||||
|
|||||||
Reference in New Issue
Block a user