Skip to content

refactor: reduce complexity of execute_safe_output in src/execute.rs#307

Merged
jamesadevine merged 4 commits intomainfrom
refactor/reduce-execute-safe-output-complexity-e770d6360ee7e02e
Apr 23, 2026
Merged

refactor: reduce complexity of execute_safe_output in src/execute.rs#307
jamesadevine merged 4 commits intomainfrom
refactor/reduce-execute-safe-output-complexity-e770d6360ee7e02e

Conversation

@github-actions
Copy link
Copy Markdown
Contributor

What was complex

execute_safe_output dispatched to 17 safe-output tools via a match statement where every arm repeated the identical 4-step pattern:

  1. debug!("Parsing <tool> payload")
  2. serde_json::from_value(...).map_err(...)?
  3. per-field debug! log
  4. output.execute_sanitized(ctx).await?

With 17 arms × multiple statements each, Clippy reported a cognitive complexity of 58/25 — more than double the threshold.

What changed

New private helper: dispatch_tool<T>

async fn dispatch_tool<T>(
    tool_name: &str,
    entry: &Value,
    ctx: &ExecutionContext,
) -> Result<ExecutionResult>
where
    T: DeserializeOwned + Executor,
{
    debug!("Parsing {} payload", tool_name);
    let mut output: T = serde_json::from_value(entry.clone())
        .map_err(|e| anyhow::anyhow!("Failed to parse {}: {}", tool_name, e))?;
    output.execute_sanitized(ctx).await
}

Simplified match arms

All 17 standard tool arms are now one-liners:

"create-work-item"    => dispatch_tool::<CreateWorkItemResult>(tool_name, entry, ctx).await?,
"comment-on-work-item" => dispatch_tool::<CommentOnWorkItemResult>(tool_name, entry, ctx).await?,
// ...

Merged informational arms

noop, missing-tool, and missing-data were three identical arms; collapsed into one:

"noop" | "missing-tool" | "missing-data" => {
    debug!("Skipping informational entry: {}", tool_name);
    ExecutionResult::success(format!("Skipped informational output: {}", tool_name))
}

report-incomplete remains a special case (does not implement Executor).

Before / After

Metric Before After
Cognitive complexity 58/25 ⚠️ below 25 ✅
Lines in function ~200 ~40
src/execute.rs net change −128 lines

Verification

  • cargo build
  • cargo test ✅ (all tests pass)
  • cargo clippy --all-targets --all-features ✅ (no warnings)
  • execute_safe_output no longer flagged by clippy::cognitive_complexity

Warning

⚠️ Firewall blocked 1 domain

The following domain was blocked by the firewall during workflow execution:

  • dev.azure.com

To allow these domains, add them to the network.allowed list in your workflow frontmatter:

network:
  allowed:
    - defaults
    - "dev.azure.com"

See Network Configuration for more information.

Generated by Cyclomatic Complexity Reducer · ● 1.8M ·

Extract generic dispatch_tool<T> helper to replace 17 near-identical
match arms, each of which repeated the same parse-then-execute pattern.
Also collapse three trivially-equivalent informational arms (noop,
missing-tool, missing-data) into one.

Before: cognitive complexity 58/25
After:  below threshold (not flagged by Clippy)

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copy link
Copy Markdown
Collaborator

@jamesadevine jamesadevine left a comment

Choose a reason for hiding this comment

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

/rust-review

Comment thread src/execute.rs Outdated
"upload-attachment" => dispatch_tool::<UploadAttachmentResult>(tool_name, entry, ctx).await?,
"submit-pr-review" => dispatch_tool::<SubmitPrReviewResult>(tool_name, entry, ctx).await?,
"reply-to-pr-review-comment" => dispatch_tool::<ReplyToPrCommentResult>(tool_name, entry, ctx).await?,
"resolve-pr-thread" => dispatch_tool::<ResolvePrThreadResult>(tool_name, entry, ctx).await?,
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

@copilot Anyway we could clean this up further with a macro approach instead? Is there something clever we could do with serde to deserialize the SafeOutputs by name automatically?

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.

Implemented a macro-backed dispatch table in execute_safe_output to reduce the remaining repetitive tool-name/type mapping (6f8c02f). I also adjusted it to return Result<Option<_>> instead of using ? inside the macro body for clearer error flow. For serde auto-deserialization by name, we could model a tagged enum, but that would require broader per-tool type reshaping; this keeps the change minimal and localized.

No UI changes in this PR, so there isn’t a UI screenshot to share.

@jamesadevine
Copy link
Copy Markdown
Collaborator

/rust-review

@github-actions
Copy link
Copy Markdown
Contributor Author

github-actions Bot commented Apr 23, 2026

Rust PR Reviewer completed successfully!

@github-actions
Copy link
Copy Markdown
Contributor Author

🔍 Rust PR Review

Summary: Looks good — clean, correct refactor with one minor observability tradeoff worth noting.

Findings

⚠️ Suggestions

  • src/execute.rs:229-232 — field-level debug logs removed: The old arms logged tool-specific fields before dispatch (e.g., debug!("create-pull-request: title='{}', repo='{}', branch='{}', patch='{}'", ...), debug!("add-pr-comment: pr_id={}, content length={}", ...)). The new dispatch_tool only logs "Parsing {} payload". When Stage 3 fails or behaves unexpectedly in production, those per-field logs were the first signal for diagnosing which payload values were actually received.

    This isn't a bug, and the tradeoff is acceptable for this refactor — but worth knowing that RUST_LOG=debug on Stage 3 is now slightly less informative for tools with interesting fields. The most impactful losses are create-pull-request (title, repo, branch, patch file) and update-pr (pr_id, operation).

✅ What Looks Good

  • Correctness: dispatch_tool preserves the exact same semantics as every old arm: entry.clone(), deserialize, execute_sanitized. The let mut output binding is correctly required because execute_sanitized takes &mut self (to run sanitize_content_fields before dispatch).
  • report-incomplete handling: Correctly kept inline since ReportIncompleteResult doesn't implement Executor, and sanitize_content_fields() is still called before the debug log.
  • Merged informational arms (noop | missing-tool | missing-data): The switch from three &'static str literals to a single format!() produces identical output strings — no behavior change.
  • Error messages: The new anyhow::anyhow!("Failed to parse {}: {}", tool_name, e) format is identical to the old per-arm messages, so operator-visible error strings are unchanged.
  • No new allocations: entry.clone() was already present in every old arm; no regression.

Generated by Rust PR Reviewer for issue #307 · ● 469.4K ·

Copy link
Copy Markdown
Contributor

Copilot AI commented Apr 23, 2026

Just as a heads up, I was blocked by some firewall rules while working on your feedback. Expand below for details.

Warning

Firewall rules blocked me from connecting to one or more addresses (expand for details)

I tried to connect to the following addresses, but was blocked by firewall rules:

  • dev.azure.com
    • Triggering command: /home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/ado_aw-d5652daa30dfc52e /home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/ado_aw-d5652daa30dfc52e /home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/ado_aw-beb2180dce8ef554.0ikdli/home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/compiler_tests-d461b4685455015d.1ket8gxh7k8pnfuz3gzg6yfzy.1ulaqjy.rcgu.o /home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/ado_aw-beb2180dce8ef554.0ixq20/home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/compiler_tests-d461b4685455015d.1n00vrauz0tjinbjhe3cmnsgl.1ulaqjy.rcgu.o /home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/ado_aw-beb2180dce8ef554.0jlbih/home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/compiler_tests-d461b4685455015d.1saraw1npdjk7qzm3nb8awhct.1ulaqjy.rcgu.o /home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/ado_aw-beb2180dce8ef554.0k0fxh/home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/compiler_tests-d461b4685455015d.20jt5s1ik51kdhvbu4hrvcifw.1ulaqjy.rcgu.o /home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/ado_aw-beb2180dce8ef554.0kwm7w/home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/compiler_tests-d461b4685455015d.2370ib6euso0f5xcqo9mphght.1ulaqjy.rcgu.o d.0oo30tlv22o5mbt2bzyaecx62.0pq44hk.rcgu.o d.0ppm7acpzj9pose041bmuqgpm.0pq44hk.rcgu.o d.0tviedpuoz1b291l92tcn0dx4.0pq44hk.rcgu.o d.0vm8jrl97m68uvl00ag7gz1p5.0pq44hk.rcgu.o d.0ybq5glinpj9818of2ja1npyy.0pq44hk.rcgu.o d.11ql65wk2qkw8dv87qp088o22.0pq44hk.rcgu.o d.14722s5epvjae3nn3ylxkucko.0pq44hk.rcgu.o d.148cy3pq04g1un12o6x1quzon.0pq44hk.rcgu.o d.17tgrokgra7a7wujk7ooupq97.0pq44hk.rcgu.o d.17vd3t69cj94hlwno3z871y8p.0pq44hk.rcgu.o d.18wuz8jn1878w063h8qkflpn2.0pq44hk.rcgu.o d.1ank73sfegqlunuub7fovwm0g.0pq44hk.rcgu.o d.1cuken4jbrqqg32ejpkaakkhq.0pq44hk.rcgu.o d.1j7uup3w3mvzupdpgrflqfbdd.0pq44hk.rcgu.o (dns block)
    • Triggering command: /home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/ado_aw-d5652daa30dfc52e /home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/ado_aw-d5652daa30dfc52e cfg(ci) --check-cfg cfg(error_generic_member_access) stup�� target/debug/deps/rustckyuXp5/symbols.o target/debug/deps/compiler_tests-d461b4685455015d.0eq5wss1qetssq8rucv9dx4vo.1ulaqjy.rcgu.o bin/rustc target/debug/dep/home/REDACTED/.rustup/toolchains/stable-x86_64-REDACTED-linux-gnu/bin/rustc target/debug/dep--crate-name d bin/rustc targ�� erms-agent.md d bin/rustc target/debug/dep/home/REDACTED/.rustup/toolchains/stable-x86_64-REDACTED-linux-gnu/bin/cargo target/debug/depbuild target/debug/dep--emit=dep-info,metadata bin/rustc (dns block)

If you need me to access, download, or install something from one of these locations, you can either:

@jamesadevine
Copy link
Copy Markdown
Collaborator

@copilot src/execute.rs:229-232 — field-level debug logs removed: The old arms logged tool-specific fields before dispatch (e.g., debug!("create-pull-request: title='{}', repo='{}', branch='{}', patch='{}'", ...), debug!("add-pr-comment: pr_id={}, content length={}", ...)). The new dispatch_tool only logs "Parsing {} payload". When Stage 3 fails or behaves unexpectedly in production, those per-field logs were the first signal for diagnosing which payload values were actually

Move the logs into each executor to compensate for lost logs.

Agent-Logs-Url: https://github.com/githubnext/ado-aw/sessions/0cb82419-b049-4b08-9430-6d959a95135e

Co-authored-by: jamesadevine <4742697+jamesadevine@users.noreply.github.com>
Copy link
Copy Markdown
Contributor

Copilot AI commented Apr 23, 2026

@copilot src/execute.rs:229-232 — field-level debug logs removed: The old arms logged tool-specific fields before dispatch (e.g., debug!("create-pull-request: title='{}', repo='{}', branch='{}', pat...

Done in 02e0e09: I moved payload-level debug context into each executor (execute_impl) and restored tool-specific field logs there (including create-pull-request and add-pr-comment). This keeps centralized dispatch simple while preserving Stage 3 diagnostics.

No UI changes were made, so there is no UI screenshot.

@jamesadevine jamesadevine marked this pull request as ready for review April 23, 2026 06:54
@jamesadevine jamesadevine merged commit 1dac259 into main Apr 23, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants