Skip to content

Commit aacde87

Browse files
refactor: reduce complexity of execute_safe_outputs in execute.rs (#313)
Extract four focused helpers to bring cognitive complexity from 34 down to below the 25 threshold: - log_execution_context: pulls out the debug-logging block including the conditional allowed-repositories list; removes one branch from the main function body. - log_queued_entries: isolates the pre-execution for+if-let loop that enumerates queued entries at debug level. - enforce_budget: flattens the previously triply-nested if-let chain for budget enforcement into a ?-chained helper that mutates the counter in place and returns Option<ExecutionResult>. - log_and_print_entry_result: moves the if/else-if/else result-logging block out of the deeply nested match Ok arm. The main loop is now five lines of straightforward sequencing rather than a deeply nested tangle. Behaviour is identical; all tests pass. Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
1 parent df08f90 commit aacde87

1 file changed

Lines changed: 71 additions & 84 deletions

File tree

src/execute.rs

Lines changed: 71 additions & 84 deletions
Original file line numberDiff line numberDiff line change
@@ -30,35 +30,7 @@ pub async fn execute_safe_outputs(
3030
) -> Result<Vec<ExecutionResult>> {
3131
let safe_output_path = safe_output_dir.join(SAFE_OUTPUT_FILENAME);
3232

33-
info!("Stage 3 execution starting");
34-
debug!("Safe output directory: {}", safe_output_dir.display());
35-
debug!("Source directory: {}", ctx.source_directory.display());
36-
debug!(
37-
"ADO org: {}",
38-
ctx.ado_org_url.as_deref().unwrap_or("<not set>")
39-
);
40-
debug!(
41-
"ADO project: {}",
42-
ctx.ado_project.as_deref().unwrap_or("<not set>")
43-
);
44-
debug!(
45-
"Repository ID: {}",
46-
ctx.repository_id.as_deref().unwrap_or("<not set>")
47-
);
48-
debug!(
49-
"Repository name: {}",
50-
ctx.repository_name.as_deref().unwrap_or("<not set>")
51-
);
52-
if !ctx.allowed_repositories.is_empty() {
53-
debug!(
54-
"Allowed repositories: {}",
55-
ctx.allowed_repositories
56-
.keys()
57-
.cloned()
58-
.collect::<Vec<_>>()
59-
.join(", ")
60-
);
61-
}
33+
log_execution_context(safe_output_dir, ctx);
6234

6335
if !safe_output_path.exists() {
6436
info!(
@@ -86,12 +58,7 @@ pub async fn execute_safe_outputs(
8658
info!("Found {} safe output(s) to execute", entries.len());
8759
println!("Found {} safe output(s) to execute", entries.len());
8860

89-
// Log summary of what we're about to execute
90-
for (i, entry) in entries.iter().enumerate() {
91-
if let Some(name) = entry.get("name").and_then(|n| n.as_str()) {
92-
debug!("[{}/{}] Queued: {}", i + 1, entries.len(), name);
93-
}
94-
}
61+
log_queued_entries(&entries);
9562

9663
// Build budget map: tool_name → (executed_count, max_allowed).
9764
// Each tool declares its DEFAULT_MAX via the ToolResult trait; the operator can
@@ -133,63 +100,19 @@ pub async fn execute_safe_outputs(
133100
let mut results = Vec::new();
134101
for (i, entry) in entries.iter().enumerate() {
135102
let entry_json = serde_json::to_string(entry).unwrap_or_else(|_| "<invalid>".to_string());
136-
debug!(
137-
"[{}/{}] Executing entry: {}",
138-
i + 1,
139-
entries.len(),
140-
entry_json
141-
);
103+
debug!("[{}/{}] Executing entry: {}", i + 1, entries.len(), entry_json);
142104

143105
// Generic budget enforcement: skip excess entries rather than aborting the whole batch.
144106
// Budget is consumed before execution so that failed attempts (target policy rejection,
145107
// network errors) still count — this prevents unbounded retries against a failing endpoint.
146-
if let Some(tool_name) = entry.get("name").and_then(|n| n.as_str()) {
147-
if let Some((executed, max)) = budgets.get_mut(tool_name) {
148-
let context_id = extract_entry_context(entry);
149-
if let Some(result) = check_budget(entries.len(), i, tool_name, &context_id, *executed, *max) {
150-
results.push(result);
151-
continue;
152-
}
153-
*executed += 1;
154-
}
108+
if let Some(result) = enforce_budget(entry, &mut budgets, entries.len(), i) {
109+
results.push(result);
110+
continue;
155111
}
156112

157113
match execute_safe_output(entry, ctx).await {
158114
Ok((tool_name, result)) => {
159-
if result.is_warning() {
160-
warn!(
161-
"[{}/{}] {} warning: {}",
162-
i + 1,
163-
entries.len(),
164-
tool_name,
165-
result.message
166-
);
167-
} else if result.success {
168-
info!(
169-
"[{}/{}] {} succeeded: {}",
170-
i + 1,
171-
entries.len(),
172-
tool_name,
173-
result.message
174-
);
175-
} else {
176-
warn!(
177-
"[{}/{}] {} failed: {}",
178-
i + 1,
179-
entries.len(),
180-
tool_name,
181-
result.message
182-
);
183-
}
184-
let symbol = if result.is_warning() { "⚠" } else if result.success { "✓" } else { "✗" };
185-
println!(
186-
"[{}/{}] {} - {} - {}",
187-
i + 1,
188-
entries.len(),
189-
tool_name,
190-
symbol,
191-
result.message
192-
);
115+
log_and_print_entry_result(i, entries.len(), &tool_name, &result);
193116
results.push(result);
194117
}
195118
Err(e) => {
@@ -213,6 +136,70 @@ pub async fn execute_safe_outputs(
213136
Ok(results)
214137
}
215138

139+
/// Emit debug-level context about the execution environment at Stage 3 startup.
140+
fn log_execution_context(safe_output_dir: &Path, ctx: &ExecutionContext) {
141+
info!("Stage 3 execution starting");
142+
debug!("Safe output directory: {}", safe_output_dir.display());
143+
debug!("Source directory: {}", ctx.source_directory.display());
144+
debug!("ADO org: {}", ctx.ado_org_url.as_deref().unwrap_or("<not set>"));
145+
debug!("ADO project: {}", ctx.ado_project.as_deref().unwrap_or("<not set>"));
146+
debug!("Repository ID: {}", ctx.repository_id.as_deref().unwrap_or("<not set>"));
147+
debug!("Repository name: {}", ctx.repository_name.as_deref().unwrap_or("<not set>"));
148+
if !ctx.allowed_repositories.is_empty() {
149+
debug!(
150+
"Allowed repositories: {}",
151+
ctx.allowed_repositories
152+
.keys()
153+
.cloned()
154+
.collect::<Vec<_>>()
155+
.join(", ")
156+
);
157+
}
158+
}
159+
160+
/// Log each queued entry at debug level before execution begins.
161+
fn log_queued_entries(entries: &[Value]) {
162+
for (i, entry) in entries.iter().enumerate() {
163+
if let Some(name) = entry.get("name").and_then(|n| n.as_str()) {
164+
debug!("[{}/{}] Queued: {}", i + 1, entries.len(), name);
165+
}
166+
}
167+
}
168+
169+
/// Check the per-tool budget for an entry.
170+
///
171+
/// Returns `Some(result)` when the budget is exhausted (caller should push the result and
172+
/// skip execution). When a slot is available the counter is incremented and `None` is
173+
/// returned so execution can proceed.
174+
fn enforce_budget(
175+
entry: &Value,
176+
budgets: &mut HashMap<&str, (usize, usize)>,
177+
total: usize,
178+
i: usize,
179+
) -> Option<ExecutionResult> {
180+
let tool_name = entry.get("name").and_then(|n| n.as_str())?;
181+
let (executed, max) = budgets.get_mut(tool_name)?;
182+
let context_id = extract_entry_context(entry);
183+
if let Some(result) = check_budget(total, i, tool_name, &context_id, *executed, *max) {
184+
return Some(result);
185+
}
186+
*executed += 1;
187+
None
188+
}
189+
190+
/// Log and print the outcome of a single safe-output execution.
191+
fn log_and_print_entry_result(i: usize, total: usize, tool_name: &str, result: &ExecutionResult) {
192+
if result.is_warning() {
193+
warn!("[{}/{}] {} warning: {}", i + 1, total, tool_name, result.message);
194+
} else if result.success {
195+
info!("[{}/{}] {} succeeded: {}", i + 1, total, tool_name, result.message);
196+
} else {
197+
warn!("[{}/{}] {} failed: {}", i + 1, total, tool_name, result.message);
198+
}
199+
let symbol = if result.is_warning() { "⚠" } else if result.success { "✓" } else { "✗" };
200+
println!("[{}/{}] {} - {} - {}", i + 1, total, tool_name, symbol, result.message);
201+
}
202+
216203
/// Parse a JSON entry as `T` and run it through `execute_sanitized`.
217204
///
218205
/// This is the common path for all tools that implement `Executor`. The tool name

0 commit comments

Comments
 (0)