refactor: reduce complexity of execute_impl in upload_build_attachment.rs

[github-actions]

Summary

Clippy's too_many_lines lint flagged UploadBuildAttachmentResult::execute_impl at 267/100 lines — the highest-complexity function in src/safe_outputs/ not already addressed in a prior refactor pass (per cache-memory history: create_pull_request.rs::execute_impl at 349/100 was already tackled across two prior PRs and remains the single largest outlier, but two other executors — this one and upload_workitem_attachment.rs — were the next biggest and this one was still unaddressed).

The function mixed together: build-id resolution/validation, artifact-name prefixing and allow-list checks, extension allow-list checks, attachment-type validation, staged-file path/size/directory verification, SHA-256 integrity checking, ADO context resolution (org/project/token/plan/timeline/record), URL construction, and the actual HTTP PUT + response handling — all inline in one async fn.

Changes

Extracted the following helpers (all private, no public API changes):

• resolve_effective_build_id — resolves/validates the current-run build id against the agent-supplied build_id
• resolve_final_artifact_name — applies name-prefix and validates charset/length/allow-list
• check_extension_allowed — validates the file extension against allowed-extensions
• resolve_attachment_type (free fn) — resolves/validates the attachment-type config value
• resolve_and_verify_staged_file — canonicalizes the staged path, verifies it's within the working directory, isn't a directory, and matches the recorded size/max-size
• resolve_ado_upload_context (free fn) — resolves org URL, project, token, project id, plan id, timeline id, record id from ExecutionContext
• build_attachment_url (free fn) — builds the DistributedTask timeline-attachment URL
• upload_attachment — sends the PUT request and maps the response to an ExecutionResult

execute_impl itself is now a short, linear sequence of calls to these helpers, unwrapping the Result<T, ExecutionResult> pattern used throughout the codebase for validation short-circuits.

Verification

• cargo build — clean
• cargo test — 3118 tests pass (including all 42 upload_build_attachment tests), 0 failures
• cargo clippy --all-targets --all-features — clean
• Re-ran the too_many_lines clippy check scoped to this file: the lint no longer fires for execute_impl

No behavior or public API changes — this is a pure internal restructuring for readability/testability.

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 · 106.6 AIC · ⌖ 8.12 AIC · ⊞ 11.3K · ◷

[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]

✅ Compiler Contract Reviewer completed the compiler contract review.

🏗️ Compiler contract review by Compiler Contract Reviewer

[github-actions]

✅ Test Quality Sentinel completed the test quality analysis.

🧪 Test quality analysis by Test Quality Sentinel

[github-actions]

✅ PR Security Reviewer completed the security review.

🔒 Security review by PR Security Reviewer

[github-actions]

Compiler Contract Review

This PR is a pure internal refactor of UploadBuildAttachmentResult::execute_impl in src/safe_outputs/upload_build_attachment.rs — extracting private helpers (resolve_effective_build_id, resolve_final_artifact_name, check_extension_allowed, resolve_attachment_type, resolve_and_verify_staged_file, resolve_ado_upload_context, build_attachment_url, upload_attachment).

Checked against this reviewer's contracts:

• Files changed: only src/safe_outputs/upload_build_attachment.rs — no codegen/lock/registry drift is possible here (no bundle sources, no types.gen.ts/fact-catalog.gen.json, no .github/workflows/*.md/.lock.yml pairs, no tests/safe-outputs/ locks touched).
• Front-matter grammar / safe-output schema: unchanged — no new Params fields, so no new newtype-vs-String question arises. Existing identifier-bearing fields (file_path, staged_file) are untouched by this diff.
• Typed IR / extensions / runtimes: not touched.
• Generated shell: not touched.
• Behavior: line-by-line diff confirms this is a mechanical extraction — validation order, error messages, dry-run short-circuit position, and the SHA-256/size/path checks are all preserved verbatim; only control flow was reshaped from inline early-returns to Result-returning helpers.
• Docs: no public contract (docs/safe-outputs.md, docs/ir.md, AGENTS.md) needs updating for an internal-only refactor.

No compiler-contract issues found. Deferring to the Rust-quality reviewer for anything about the extraction style itself (e.g. the Result<Result<T, ExecutionResult>> nesting pattern).

🏗️ Compiler contract review by Compiler Contract Reviewer · auto · 28.1 AIC · ⌖ 2.08 AIC · ⊞ 12.7K
_{Comment /review to run again}

[github-actions]

Security review: no regressions

This is a mechanical refactor of UploadBuildAttachmentResult::execute_impl extracting helper functions (resolve_effective_build_id, resolve_final_artifact_name, check_extension_allowed, resolve_attachment_type, resolve_and_verify_staged_file, resolve_ado_upload_context, build_attachment_url, upload_attachment). I traced the diff line-by-line against the original inline logic:

• Build-id, artifact-name allow-list/charset, extension allow-list, attachment-type charset validations are preserved verbatim, just moved into named methods.
• Staged-file path canonicalization + starts_with(working_directory) containment check, directory rejection, size match, and SHA-256 integrity check against self.staged_sha256 are all unchanged and still run in the same order before any upload occurs.
• Token (SYSTEM_ACCESSTOKEN/AZURE_DEVOPS_EXT_PAT) resolution and its use via basic_auth("", Some(ado_ctx.token)) is unchanged — still only used for the outbound PUT in upload_attachment, never logged or echoed.
• URL construction still percent-encodes every path segment (project_id, plan_id, timeline_id, record_id, attachment_type, final_name) via utf8_percent_encode(..., PATH_SEGMENT) — no new unescaped interpolation introduced.
• Control flow ordering is preserved (dry-run short-circuit still happens after size/staged-file checks but before reading file bytes / hashing / network calls).

No new file operations, no new external inputs, no widened allow-lists or regexes, no token scope changes, and no new network/allowlist domains. This diff is security-neutral.

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

[github-actions]

This PR is a pure, mechanical decomposition of execute_impl — no production behavior changes, all extracted branches preserve their exact error messages/formatting, and all existing execute_impl-level tests (build-id resolution, name-prefix, extension allow-list, artifact-name allow-list, staged-file integrity, missing ADO context vars, SHA-256 mismatch) still cover the new call graph end-to-end, so nothing is newly untested at the integration level.

The gap: several of the newly-extracted helpers (resolve_attachment_type, build_attachment_url, resolve_final_artifact_name's name-prefix-too-long branch) are now pure functions with no ExecutionContext/async dependency, but no direct unit test was added for them — coverage is only indirect via execute_impl dry-run scenarios, and several rejection branches (invalid attachment-type, name-prefix >50 chars) aren't hit by any existing test at all. Left one inline comment on the most significant instance. Not blocking — this is a real regression-detection gap but the refactor itself introduces no behavior risk, so COMMENT rather than REQUEST_CHANGES.

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

[github-actions]

This refactor turns attachment-type validation into a standalone pure function taking a &UploadBuildAttachmentConfig, but no unit test exercises it (or its error path) directly — the only coverage is indirect, through execute_impl's dry-run tests, none of which set an invalid attachment-type.

💡 what's untested

resolve_attachment_type now has zero ExecutionContext/tokio dependencies, so a plain #[test] could assert each rejection reason directly:

#[test]
fn test_resolve_attachment_type_rejects_leading_dot() {
    let config = UploadBuildAttachmentConfig { attachment_type: Some(".bad".into()), ..Default::default() };
    let err = resolve_attachment_type(&config).unwrap_err();
    assert!(matches!(err, ExecutionResult { success: false, .. }));
}

Right now, if a future change (e.g. relaxing is_valid_artifact_name or the length bound) regressed one of these four rejection reasons, nothing in the suite would catch it — the only executor-level tests that touch attachment-type use the default value.

[github-actions]

Reviewed the Rust engineering quality of this refactor (extracting helpers out of UploadBuildAttachmentResult::execute_impl). This is a purely mechanical decomposition — traced each extracted function (resolve_effective_build_id, resolve_final_artifact_name, check_extension_allowed, resolve_attachment_type, resolve_and_verify_staged_file, resolve_ado_upload_context, build_attachment_url, upload_attachment) against the original inline logic and found the control flow, error messages, and ordering preserved exactly (e.g. dry-run guard still runs before the file read, SHA-256 check still happens after size checks). All 39 existing unit tests pass and cargo clippy is clean, including the new #[allow(clippy::too_many_arguments)] on upload_attachment.

No merge-blocking defects found. rust-critic sub-agent produced no usable findings (empty response after retry), so this review relies solely on my own pass.

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