refactor: reduce complexity of execute_impl in create_work_item.rs

[github-actions]

Summary

CreateWorkItemResult::execute_impl in src/safe_outputs/create_work_item.rs was flagged by clippy's too_many_lines lint at 205/100 — the second-highest unaddressed candidate in the codebase after create_pull_request.rs::execute_impl, which has already been the subject of several prior refactor attempts.

What was complex

The function mixed several distinct concerns inline:

• Tag merging (static config tags + agent-provided tags, deduped case-insensitively)
• Allowlist validation of agent-provided tags
• Building the ADO JSON Patch document (title, description, optional fields, tags, custom fields)
• Handling the HTTP success path (parsing the response, registering the resolved temporary ID, building the result message/payload)
• Handling the HTTP failure path

What changed

Extracted five well-named helper functions, each owning one concern:

• merge_tags — merges config and agent tags, case-insensitive dedup
• check_allowed_tags — validates agent tags against the allowlist, returning an Err(message) instead of inline branching
• build_patch_document — builds the full JSON Patch document, including field validation
• handle_creation_success — parses the success response, registers the temporary ID, builds the result
• handle_creation_failure — builds the failure result from a non-2xx response

execute_impl now reads as a straight-line sequence of these calls with no behavior change.

Before/after

• Original: 205/100 (too_many_lines)
• After: below the clippy too_many_lines threshold (no longer flagged)

Verification

• cargo build — clean
• cargo test (full suite) — all pass, no failures
• cargo clippy --all-targets --all-features — clean (one pre-existing, unrelated warning in test code at line 1055)
• No public API or observable behavior changes; same validation order, same error messages, same success/failure payloads.

Warning

Firewall blocked 1 domain

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

• spsprodeus21.vssps.visualstudio.com

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

network:
  allowed:
    - defaults
    - "spsprodeus21.vssps.visualstudio.com"

See Network Configuration for more information.

Generated by Cyclomatic Complexity Reducer · auto · 79.1 AIC · ⌖ 10.8 AIC · ⊞ 11.4K · ◷

[azure-pipelines]

Azure Pipelines:
Successfully started running 1 pipeline(s).
1 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

[azure-pipelines]

Azure Pipelines:
Successfully started running 1 pipeline(s).
1 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

[github-actions]

✅ Rust Code Quality Reviewer completed the Rust code quality review.

🦀 Rust code quality review by Rust Code Quality Reviewer

[github-actions]

✅ Test Quality Sentinel completed the test quality analysis.

🧪 Test quality analysis by Test Quality Sentinel

[github-actions]

✅ Compiler Contract Reviewer completed the compiler contract review.

Reviewed PR #2006 (create_work_item.rs refactor extracting 5 helper functions to reduce cyclomatic complexity). No compiler-contract violations found: no grammar/schema changes, no new safe-output tools, no IR changes, no shell script changes, no identifier fields, no codegen artifacts affected. This is a behavior-preserving internal refactor confined to src/safe_outputs/create_work_item.rs. Out of scope for compiler-contract review.

🏗️ Compiler contract review by Compiler Contract Reviewer

[github-actions]

✅ PR Security Reviewer completed the security review.

🔒 Security review by PR Security Reviewer

[github-actions]

This is a clean, behavior-preserving extraction of execute_impl into five named helpers, and the diff confirms the logic moved verbatim.

However, none of the five new functions (merge_tags, check_allowed_tags, build_patch_document, handle_creation_success, handle_creation_failure) gained a direct unit test. Extraction was the ideal moment to add fast, isolated coverage for this logic (tag dedup, allowlist matching, patch-doc assembly, success/failure response handling) instead of relying solely on indirect exercise through the full Executor::execute path. Left two inline comments on the highest-value gaps (merge_tags, check_allowed_tags).

Not blocking since behavior is unchanged and the existing test suite still passes, but the PR body's claim of "no behavior change" would be stronger with new tests pinning that behavior at the function level.

🧪 Test quality analysis by Test Quality Sentinel · auto · 31.4 AIC · ⌖ 2.25 AIC · ⊞ 11.2K
_{Comment /review to run again}

Comments that could not be inline-anchored

src/safe_outputs/create_work_item.rs:19

merge_tags has no unit test. This is trivially testable pure logic (case-insensitive dedup across two slices) that is now a named, independently reachable function — a future edit to the dedup rule would ship silently.

<details><summary>💡 suggested test</summary>

#[test]
fn test_merge_tags_dedups_case_insensitively() {
    let merged = merge_tags(&["Foo".into()], &["foo".into(), "bar".into()]);
    assert_eq!(merged, vec!["Foo".to_string(), "bar".to_string()]);
}

</details>

src/safe_outputs/create_work_item.rs:47

check_allowed_tags is now an independently callable function but has no direct unit test — only indirect coverage via the executor path. Its early-return-Ok when either list is empty, and the pattern-matching branch (tag_matches_pattern), are both untested in isolation.

<details><summary>💡 gap</summary>

Nothing here exercises the case where allowed_tags is non-empty but every agent tag matches (should be Ok(())), nor the message-format branch when multiple tags are disallowed (comma-jo…

[github-actions]

Security review: no regression found

This PR is a pure mechanical refactor of CreateWorkItemResult::execute_impl in src/safe_outputs/create_work_item.rs, extracting five helper functions (merge_tags, check_allowed_tags, build_patch_document, handle_creation_success, handle_creation_failure). Compared line-by-line against the original inline logic:

• Tag allowlist validation (check_allowed_tags) preserves the exact same tag_matches_pattern check and disallowed-tag error message.
• build_patch_document preserves the same validate_patch_fields call, field ordering, and assignee normalization via normalize_work_item_assignee.
• handle_creation_success preserves the same response parsing, register_resolved_work_item call, and — critically — the same crate::sanitize::neutralize_pipeline_commands call on the error string before embedding it in the result message (no sanitization bypass).
• No new user-controlled input, no new file/path handling, no new tokens/secrets, no new network destinations, and no change to the Stage 1/Stage 3 safe-output boundary (results are still returned for later application, not applied directly).

No security regression, injection vector, weakened validation, or token-scope change found in this diff.

🔒 Security review by PR Security Reviewer · auto · 20.5 AIC · ⌖ 4.49 AIC · ⊞ 11.7K
_{Comment /review to run again}

[github-actions]

Verdict: COMMENT — no blocking issues

Reviewed the refactor of CreateWorkItemResult::execute_impl in src/safe_outputs/create_work_item.rs (extraction into merge_tags, check_allowed_tags, build_patch_document, handle_creation_success, handle_creation_failure).

I diffed the extracted helper bodies against the original inline logic line-by-line: control flow, error propagation (Result<_, String> via .map_err/?), field ordering in the JSON Patch document, and the success/failure payload shapes are all preserved exactly. No behavior change, no new unwrap()/expect() on fallible paths, no lossy casts, no ordering-sensitive iteration introduced. The helpers are appropriately scoped as private free functions with reasonable parameter lists, and doc comments were added for each.

💡 Details

• merge_tags: identical dedup logic (case-insensitive eq_ignore_ascii_case), same push order.
• check_allowed_tags: identical early-return guards and disallowed filter/format, now returning Err(String) instead of inline return Ok(ExecutionResult::failure(...)) — call site correctly maps this back to the same ExecutionResult::failure.
• build_patch_document: description_field_for/validate_patch_fields ordering unchanged; all optional fields (area_path, iteration_path, assignee, tags, custom fields) appended in the same order as before.
• handle_creation_success/handle_creation_failure: byte-for-byte equivalent to the original inline arms, just parameterized.

Note: the rust-critic sub-agent didn't return usable output after two follow-up prompts (empty responses), so this review relies solely on my own pass — flagging per the review contract.

No merge-blocking findings; nothing to comment inline on.

🦀 Rust code quality review by Rust Code Quality Reviewer · auto · 37.4 AIC · ⌖ 2.26 AIC · ⊞ 11.5K
_{Comment /review to run again}