[codex] Repair invalid skill frontmatter scalars (#28628)

## Why The community marketplace audit found many skill frontmatter parse failures where values were intended as prose, but were not valid YAML. Common examples include unquoted scalar values with `: `, such as `description: Build for AWS: ECS` or `argument-hint: <duration: e.g. 7d>`, and flow-looking values such as `tags: [next,@supabase/ssr]`. `serde_yaml` does not expose a permissive mode for this. The parser fails before unknown frontmatter fields can be ignored, so a compatibility repair has to happen before retrying YAML parsing. ## What changed Skill frontmatter loading still uses `serde_yaml` as the primary parser. If that parse fails, the loader performs a line-oriented repair of scalar frontmatter field values, then retries parsing. The fallback now: - applies to any frontmatter mapping field, not just `description` / `short-description` - quotes unquoted scalar values that contain a YAML colon separator such as `: ` - quotes invalid flow-looking scalar values that start with `[`, `{`, `@`, or backtick - preserves already quoted values - skips `|` / `>` block scalar bodies so multiline descriptions are not rewritten - returns the original YAML error if the repaired frontmatter still cannot parse ## Examples This previously failed because the second `: ` was parsed as YAML structure: ```yaml description: AWS deployment patterns: ECS Fargate, Lambda, and S3 ``` The fallback now parses it as if it had been written explicitly as: ```yaml description: 'AWS deployment patterns: ECS Fargate, Lambda, and S3' ``` The same repair now applies to ignored frontmatter fields that still need to be valid YAML for the parser to get through the document: ```yaml argument-hint: <duration: e.g. 7d, 2w> tags: [next,@supabase/ssr] ``` Valid YAML multiline descriptions continue to work through normal parsing without repair: ```yaml description: |- Build for AWS: ECS and Lambda ``` ## Validation - Added loader coverage for unquoted `description` values containing `: `. - Added loader coverage for unquoted `metadata.short-description` values containing `: ` and an apostrophe. - Added loader coverage for unrecognized frontmatter fields that need quoting, including `argument-hint` and `tags`. - Added block-scalar coverage to ensure multiline description bodies are preserved while other fields are repaired. - `just test -p codex-core-skills` (106 passed) - `just fix -p codex-core-skills`
2026-07-01 00:31:56 +08:00 · 2026-06-17 10:37:14 -07:00
parent ef75171f18
commit 0065c3a17d
2 changed files with 226 additions and 2 deletions
@@ -651,8 +651,19 @@ async fn parse_skill_file(

    let frontmatter = extract_frontmatter(&contents).ok_or(SkillParseError::MissingFrontmatter)?;

-    let parsed: SkillFrontmatter =
-        serde_yaml::from_str(&frontmatter).map_err(SkillParseError::InvalidYaml)?;
+    let parsed: SkillFrontmatter = match serde_yaml::from_str(&frontmatter) {
+        Ok(parsed) => Ok(parsed),
+        Err(original_error) => match repair_frontmatter_scalar_fields(&frontmatter) {
+            // Some third-party skills use prose like `description: Build for AWS: ECS`
+            // or `argument-hint: <duration: e.g. 7d>`. Keep the repair line-oriented
+            // so unrelated invalid YAML still surfaces.
+            Some(repaired_frontmatter) => {
+                serde_yaml::from_str(&repaired_frontmatter).map_err(|_| original_error)
+            }
+            None => Err(original_error),
+        },
+    }
+    .map_err(SkillParseError::InvalidYaml)?;

    let base_name = parsed
        .name
@@ -997,6 +1008,91 @@ fn sanitize_single_line(raw: &str) -> String {
    raw.split_whitespace().collect::<Vec<_>>().join(" ")
 }

+fn repair_frontmatter_scalar_fields(frontmatter: &str) -> Option<String> {
+    let mut changed = false;
+    let mut block_scalar_indent: Option<usize> = None;
+    let mut repaired_lines: Vec<String> = Vec::new();
+    for line in frontmatter.lines() {
+        let indent = line
+            .chars()
+            .take_while(|character| *character == ' ')
+            .count();
+        if let Some(block_indent) = block_scalar_indent {
+            if line.trim().is_empty() || indent > block_indent {
+                repaired_lines.push(line.to_string());
+                continue;
+            }
+            block_scalar_indent = None;
+        }
+
+        let Some((key, value)) = line.split_once(':') else {
+            repaired_lines.push(line.to_string());
+            continue;
+        };
+        if key.trim().is_empty() || !value.chars().next().is_none_or(char::is_whitespace) {
+            repaired_lines.push(line.to_string());
+            continue;
+        }
+
+        let trimmed_start = value.trim_start();
+        let leading_whitespace = &value[..value.len() - trimmed_start.len()];
+        let mut scalar = trimmed_start;
+        let mut comment = "";
+        for (index, character) in trimmed_start.char_indices() {
+            if character == '#'
+                && (index == 0
+                    || trimmed_start[..index]
+                        .chars()
+                        .next_back()
+                        .is_some_and(char::is_whitespace))
+            {
+                let comment_start = trimmed_start[..index].trim_end().len();
+                scalar = &trimmed_start[..comment_start];
+                comment = &trimmed_start[comment_start..];
+                break;
+            }
+        }
+
+        let scalar = scalar.trim_end();
+        let Some(first_char) = scalar.chars().next() else {
+            repaired_lines.push(line.to_string());
+            continue;
+        };
+        if matches!(first_char, '|' | '>') {
+            block_scalar_indent = Some(indent);
+            repaired_lines.push(line.to_string());
+            continue;
+        }
+        if matches!(first_char, '\'' | '"') {
+            repaired_lines.push(line.to_string());
+            continue;
+        }
+        let mut has_colon_separator = false;
+        let mut chars = scalar.chars().peekable();
+        while let Some(character) = chars.next() {
+            if character == ':'
+                && matches!(chars.peek(), Some(next_character) if next_character.is_whitespace())
+            {
+                has_colon_separator = true;
+                break;
+            }
+        }
+        let invalid_flow_like_scalar = matches!(first_char, '[' | '{' | '@' | '`')
+            && serde_yaml::from_str::<serde_yaml::Value>(scalar).is_err();
+        if !has_colon_separator && !invalid_flow_like_scalar {
+            repaired_lines.push(line.to_string());
+            continue;
+        }
+
+        let quoted_scalar = format!("'{}'", scalar.replace('\'', "''"));
+        repaired_lines.push(format!(
+            "{key}:{leading_whitespace}{quoted_scalar}{comment}"
+        ));
+        changed = true;
+    }
+    changed.then(|| repaired_lines.join("\n"))
+}
+
 fn validate_len(
    value: &str,
    max_len: usize,
@@ -1411,6 +1411,134 @@ async fn loads_short_description_from_metadata() {
    );
 }

+#[tokio::test]
+async fn loads_unquoted_description_containing_colon_space() {
+    let codex_home = tempfile::tempdir().expect("tempdir");
+    let skill_path = write_raw_skill_at(
+        &codex_home.path().join("skills"),
+        "colon-description",
+        "name: colon-description\ndescription: AWS deployment patterns: ECS Fargate, Lambda, and S3",
+    );
+
+    let cfg = make_config(&codex_home).await;
+    let outcome = load_skills_for_test(&cfg).await;
+    assert!(
+        outcome.errors.is_empty(),
+        "unexpected errors: {:?}",
+        outcome.errors
+    );
+    assert_eq!(
+        outcome.skills,
+        vec![SkillMetadata {
+            name: "colon-description".to_string(),
+            description: "AWS deployment patterns: ECS Fargate, Lambda, and S3".to_string(),
+            short_description: None,
+            interface: None,
+            dependencies: None,
+            policy: None,
+            path_to_skills_md: normalized(&skill_path),
+            scope: SkillScope::User,
+            plugin_id: None,
+        }]
+    );
+}
+
+#[tokio::test]
+async fn loads_unquoted_short_description_containing_colon_space_and_apostrophe() {
+    let codex_home = tempfile::tempdir().expect("tempdir");
+    let skill_path = write_raw_skill_at(
+        &codex_home.path().join("skills"),
+        "colon-short-description",
+        "name: colon-short-description\ndescription: long description\nmetadata:\n  short-description: What's included: builds and tests",
+    );
+
+    let cfg = make_config(&codex_home).await;
+    let outcome = load_skills_for_test(&cfg).await;
+    assert!(
+        outcome.errors.is_empty(),
+        "unexpected errors: {:?}",
+        outcome.errors
+    );
+    assert_eq!(
+        outcome.skills,
+        vec![SkillMetadata {
+            name: "colon-short-description".to_string(),
+            description: "long description".to_string(),
+            short_description: Some("What's included: builds and tests".to_string()),
+            interface: None,
+            dependencies: None,
+            policy: None,
+            path_to_skills_md: normalized(&skill_path),
+            scope: SkillScope::User,
+            plugin_id: None,
+        }]
+    );
+}
+
+#[tokio::test]
+async fn loads_unrecognized_frontmatter_fields_that_need_quotes() {
+    let codex_home = tempfile::tempdir().expect("tempdir");
+    let skill_path = write_raw_skill_at(
+        &codex_home.path().join("skills"),
+        "repaired-unknown-fields",
+        "name: repaired-unknown-fields\ndescription: valid description\nargument-hint: <duration: e.g. 7d, 2w>\ntags: [next,@supabase/ssr]",
+    );
+
+    let cfg = make_config(&codex_home).await;
+    let outcome = load_skills_for_test(&cfg).await;
+    assert!(
+        outcome.errors.is_empty(),
+        "unexpected errors: {:?}",
+        outcome.errors
+    );
+    assert_eq!(
+        outcome.skills,
+        vec![SkillMetadata {
+            name: "repaired-unknown-fields".to_string(),
+            description: "valid description".to_string(),
+            short_description: None,
+            interface: None,
+            dependencies: None,
+            policy: None,
+            path_to_skills_md: normalized(&skill_path),
+            scope: SkillScope::User,
+            plugin_id: None,
+        }]
+    );
+}
+
+#[tokio::test]
+async fn preserves_block_scalar_body_while_repairing_other_fields() {
+    let codex_home = tempfile::tempdir().expect("tempdir");
+    let skill_path = write_raw_skill_at(
+        &codex_home.path().join("skills"),
+        "block-description-with-repair",
+        "name: block-description-with-repair\ndescription: |-\n  Build for AWS: ECS\nargument-hint: <duration: e.g. 7d>",
+    );
+
+    let cfg = make_config(&codex_home).await;
+    let outcome = load_skills_for_test(&cfg).await;
+    assert!(
+        outcome.errors.is_empty(),
+        "unexpected errors: {:?}",
+        outcome.errors
+    );
+    assert_eq!(
+        outcome.skills,
+        vec![SkillMetadata {
+            name: "block-description-with-repair".to_string(),
+            description: "Build for AWS: ECS".to_string(),
+            short_description: None,
+            interface: None,
+            dependencies: None,
+            policy: None,
+            path_to_skills_md: normalized(&skill_path),
+            scope: SkillScope::User,
+            plugin_id: None,
+        }]
+    );
+}
+
 #[tokio::test]
 async fn enforces_short_description_length_limits() {
    let codex_home = tempfile::tempdir().expect("tempdir");