Skip to content
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

175 changes: 175 additions & 0 deletions crates/iii-worker/src/cli/config_file.rs
Original file line number Diff line number Diff line change
Expand Up @@ -139,6 +139,50 @@ fn remove_worker_from(content: &str, name: &str) -> String {
out
}

/// Rewrites a standalone `workers: []` line to `workers:` so subsequent list
/// items can be appended without producing invalid YAML.
///
/// Only touches lines where the `workers:` key is followed by an inline empty
/// list marker (`[]`, with tolerated surrounding whitespace). Populated inline
/// lists (e.g. `workers: [foo]`) and normal block lists are left untouched.
/// Trailing `# comment` text is preserved.
fn normalize_empty_workers_list(content: &str) -> String {
let lines: Vec<&str> = content.lines().collect();
let mut out: Vec<String> = Vec::with_capacity(lines.len());

for line in &lines {
let trimmed_start = line.trim_start();
let indent = &line[..line.len() - trimmed_start.len()];
if let Some(rest) = trimmed_start.strip_prefix("workers:") {
// Split off a trailing comment before checking the marker shape,
// so `workers: [] # comment` still matches.
let (value, comment) = match rest.find('#') {
Some(idx) => (&rest[..idx], Some(&rest[idx..])),
None => (rest, None),
};
let trimmed = value.trim();
let is_empty_inline_list = trimmed.starts_with('[')
&& trimmed.ends_with(']')
&& trimmed.len() >= 2
&& trimmed[1..trimmed.len() - 1].trim().is_empty();
if is_empty_inline_list {
match comment {
Some(c) => out.push(format!("{}workers: {}", indent, c.trim_start())),
None => out.push(format!("{}workers:", indent)),
}
continue;
}
}
out.push((*line).to_string());
Comment on lines +153 to +176
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Restrict normalization scope to the registry workers key to avoid config mutation.

normalize_empty_workers_list rewrites any workers: [] line at any indentation depth. Since Line 508 runs it unconditionally before every append, nested user config (e.g., under config:) can be silently changed from empty list to null.

💡 Suggested patch
 fn normalize_empty_workers_list(content: &str) -> String {
     let lines: Vec<&str> = content.lines().collect();
+    // Only normalize the shallowest `workers:` key(s), so nested config keys
+    // like `config.workers: []` are not rewritten.
+    let min_workers_indent = lines
+        .iter()
+        .filter_map(|line| {
+            let trimmed = line.trim_start();
+            if trimmed.starts_with("workers:") {
+                Some(line.len() - trimmed.len())
+            } else {
+                None
+            }
+        })
+        .min();
+
     let mut out: Vec<String> = Vec::with_capacity(lines.len());

     for line in &lines {
         let trimmed_start = line.trim_start();
-        let indent = &line[..line.len() - trimmed_start.len()];
-        if let Some(rest) = trimmed_start.strip_prefix("workers:") {
+        let indent_len = line.len() - trimmed_start.len();
+        let indent = &line[..indent_len];
+        if Some(indent_len) == min_workers_indent {
+            if let Some(rest) = trimmed_start.strip_prefix("workers:") {
             // Split off a trailing comment before checking the marker shape,
             // so `workers: [] # comment` still matches.
             let (value, comment) = match rest.find('#') {
                 Some(idx) => (&rest[..idx], Some(&rest[idx..])),
                 None => (rest, None),
             };
             let trimmed = value.trim();
             let is_empty_inline_list = trimmed.starts_with('[')
                 && trimmed.ends_with(']')
                 && trimmed.len() >= 2
                 && trimmed[1..trimmed.len() - 1].trim().is_empty();
             if is_empty_inline_list {
                 match comment {
                     Some(c) => out.push(format!("{}workers: {}", indent, c.trim_start())),
                     None => out.push(format!("{}workers:", indent)),
                 }
                 continue;
             }
+            }
         }
         out.push((*line).to_string());
     }

Also applies to: 506-509

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@crates/iii-worker/src/cli/config_file.rs` around lines 153 - 176,
normalize_empty_workers_list currently rewrites any "workers: []" at any
indentation; restrict it to only the top-level registry key by checking the
indentation before modifying the line: in normalize_empty_workers_list (the loop
over lines, variables lines, indent, trimmed_start, rest, etc.) only treat the
match as the top-level "workers:" when indent.is_empty() (or indent.len() == 0);
if indent is non-empty leave the line untouched and push the original line to
out so nested config under other keys is not mutated.

}

let mut joined = out.join("\n");
if content.ends_with('\n') {
joined.push('\n');
}
joined
}

/// Extract the raw YAML config block for a named worker from file content.
///
/// Returns the config lines (without the `config:` key itself) as a string
Expand Down Expand Up @@ -459,6 +503,10 @@ fn append_to_content_with_fields(
worker_path: Option<&str>,
config_yaml: Option<&str>,
) -> Result<(), String> {
// Normalize `workers: []` → `workers:` so appending list items below
// produces valid YAML.
*content = normalize_empty_workers_list(content);

// Ensure there is a `workers:` key.
if !content.contains("workers:") {
if !content.is_empty() && !content.ends_with('\n') {
Expand Down Expand Up @@ -1093,4 +1141,131 @@ mod tests {
assert!(config.is_some());
assert!(config.unwrap().contains("timeout: 30"));
}

#[test]
fn test_append_to_content_with_inline_empty_list_marker() {
// Reproduces the bug where `workers: []` (valid YAML) is populated
// in-place, producing `workers: []\n - name: foo` (invalid YAML).
let mut content = "workers: []\n".to_string();
let path = std::path::Path::new("/tmp/test-empty-list-marker.yaml");

append_to_content_with_fields(&mut content, path, "iii-state", None, None, None).unwrap();

assert!(
content.contains("- name: iii-state"),
"expected worker entry, got:\n{}",
content
);
assert!(
!content.contains("workers: []"),
"inline `[]` marker should be stripped, got:\n{}",
content
);
let parsed: serde_yaml::Value =
serde_yaml::from_str(&content).expect("output should be valid YAML");
let workers = parsed
.get("workers")
.and_then(|w| w.as_sequence())
.expect("`workers` should be a sequence");
assert_eq!(workers.len(), 1);
assert_eq!(
workers[0].get("name").and_then(|n| n.as_str()),
Some("iii-state")
);

let _ = std::fs::remove_file(path);
}

#[test]
fn test_normalize_empty_workers_list_strips_inline_marker() {
assert_eq!(normalize_empty_workers_list("workers: []\n"), "workers:\n");
assert_eq!(normalize_empty_workers_list("workers: []"), "workers:");
assert_eq!(normalize_empty_workers_list("workers: []\n"), "workers:\n");
assert_eq!(normalize_empty_workers_list("workers: [ ]\n"), "workers:\n");
assert_eq!(
normalize_empty_workers_list("workers: [] \n"),
"workers:\n"
);
}

#[test]
fn test_normalize_empty_workers_list_leaves_populated_list_alone() {
let content = "workers:\n - name: foo\n";
assert_eq!(normalize_empty_workers_list(content), content);
}

#[test]
fn test_normalize_empty_workers_list_leaves_inline_populated_list_alone() {
// Pin current behavior: inline populated lists are left untouched.
let content = "workers: [foo]\n";
assert_eq!(normalize_empty_workers_list(content), content);
}

#[test]
fn test_normalize_empty_workers_list_no_workers_key() {
let content = "other: value\n";
assert_eq!(normalize_empty_workers_list(content), content);
}

#[test]
fn test_normalize_empty_workers_list_preserves_other_content() {
let content = "before: 1\nworkers: []\nafter: 2\n";
let expected = "before: 1\nworkers:\nafter: 2\n";
assert_eq!(normalize_empty_workers_list(content), expected);
}

#[test]
fn test_normalize_empty_workers_list_strips_marker_with_trailing_comment() {
assert_eq!(
normalize_empty_workers_list("workers: [] # no workers yet\n"),
"workers: # no workers yet\n"
);
assert_eq!(
normalize_empty_workers_list("workers: []# tight\n"),
"workers: # tight\n"
);
}

#[test]
fn test_normalize_empty_workers_list_preserves_indentation() {
assert_eq!(
normalize_empty_workers_list(" workers: [] # nested\n"),
" workers: # nested\n"
);
}

#[test]
fn test_normalize_empty_workers_list_preserves_comment_on_populated_inline_list() {
// Populated inline list stays untouched, comment and all.
let content = "workers: [foo] # has one\n";
assert_eq!(normalize_empty_workers_list(content), content);
}

#[test]
fn test_append_to_content_with_inline_empty_list_marker_and_comment() {
let mut content = "workers: [] # add workers here\n".to_string();
let path = std::path::Path::new("/tmp/test-empty-list-marker-comment.yaml");

append_to_content_with_fields(&mut content, path, "iii-state", None, None, None).unwrap();

assert!(
content.contains("- name: iii-state"),
"expected worker entry, got:\n{}",
content
);
assert!(
!content.contains("workers: []"),
"inline `[]` marker should be stripped, got:\n{}",
content
);
let parsed: serde_yaml::Value =
serde_yaml::from_str(&content).expect("output should be valid YAML");
let workers = parsed
.get("workers")
.and_then(|w| w.as_sequence())
.expect("`workers` should be a sequence");
assert_eq!(workers.len(), 1);

let _ = std::fs::remove_file(path);
}
}
Loading