refactor(compile): reduce complexity of build_conclusion_job in agentic_pipeline.rs - #2037
Conversation
…ic_pipeline.rs Extracted two helper functions from build_conclusion_job: - apply_conclusion_tool_config_env: applies a single per-tool (noop/ missing-tool/missing-data) config as flat AW_<TOOL>_* env vars - hoist_conclusion_job_results: hoists upstream job results (Agent/ Detection/SafeOutputs[/_Reviewed]/custom jobs) into job-level variables and wires them onto the Conclusion step No behavior change. Reduces the too_many_lines lint from 195/100 for build_conclusion_job. Full test suite (3229 tests) and clippy pass. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
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: 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. |
|
✅ PR Security Reviewer completed the security review.
|
|
✅ Compiler Contract Reviewer completed the compiler contract review.
|
|
✅ Rust Code Quality Reviewer completed the Rust code quality review.
|
|
✅ Test Quality Sentinel completed the test quality analysis. Test Quality review: this PR is a pure, line-for-line extraction refactor of build_conclusion_job in src/compile/agentic_pipeline.rs into two helper functions (apply_conclusion_tool_config_env, hoist_conclusion_job_results). No production logic changed and no new test files were touched. Existing tests for build_conclusion_job already exercise this code path unchanged, so no additional test coverage is required. No findings.
|
There was a problem hiding this comment.
Security review: no regressions found
This is a pure mechanical refactor extracting apply_conclusion_tool_config_env and hoist_conclusion_job_results out of build_conclusion_job. Diffed line-by-line against main: the sanitize.rs calls on title-prefix/work-item-type/area-path/iteration-path are preserved unchanged, the report-as-work-item bool/string handling (avoiding the JSON-quoting inversion bug) is preserved, the $[dependencies...] runtime-expression hoist into job variables (vs. step env, per the ADO evaluation-context comment) is preserved, and no token/credential handling is touched. No new sinks, no widened validation, no behavior change. Security-neutral.
🔒 Security review by PR Security Reviewer · auto · 21.8 AIC · ⌖ 3.46 AIC · ⊞ 11.7K
Comment /review to run again
There was a problem hiding this comment.
Verdict: COMMENT — no merge-blocking issues found.
Reviewed the two extracted helpers (apply_conclusion_tool_config_env, hoist_conclusion_job_results) against the original inline logic in build_conclusion_job. This is a faithful, pure mechanical extraction:
- Control flow is preserved 1:1 (
if let/continue→let-else/early-return is behavior-equivalent). - All env var names, job-variable hoisting, and
$[dependencies...]expressions are unchanged. - No new
unwrap()/expect()introduced, error propagation via?onprefix.id(...)is preserved. rust-criticsub-agent returned no findings on this diff.
No inline comments needed.
🦀 Rust code quality review by Rust Code Quality Reviewer · auto · 25.7 AIC · ⌖ 2.23 AIC · ⊞ 11.5K
Comment /review to run again
There was a problem hiding this comment.
Pure mechanical extraction — two closures (apply_conclusion_tool_config_env, hoist_conclusion_job_results) pulled out of build_conclusion_job with identical logic and no signature/behavior changes. Verified: cargo build succeeds and a line-by-line diff shows the extracted bodies are byte-identical to the inlined originals (only indentation/braces differ). No front-matter grammar, safe-output tool, typed IR, or generated-artefact changes accompany this PR, so none of the compiler-contract drift checks apply (no .gen.ts/.gen.json changes needed since filter_ir.rs/Fact are untouched; no .lock.yml drift since no .github/workflows/*.md changed; no docs pages need updates). Nothing to flag.
🏗️ Compiler contract review by Compiler Contract Reviewer · auto · 24.4 AIC · ⌖ 2.77 AIC · ⊞ 12.7K
Comment /review to run again
What was complex
build_conclusion_jobinsrc/compile/agentic_pipeline.rswas flagged by Clippy'stoo_many_lineslint at 195/100 lines. It mixed several distinct concerns in one function body: building the Conclusion job's steps, applying per-tool (noop/missing-tool/missing-data) config as flat env vars, and hoisting upstream job results (Agent/Detection/SafeOutputs[/_Reviewed]/custom jobs) into job-level variables + step env wiring.What changed
Extracted two well-named helper functions, keeping
build_conclusion_job's public signature and behavior unchanged:apply_conclusion_tool_config_env— applies a single per-tool config (noop/missing-tool/missing-data) onto the Conclusion step as flatAW_<TOOL>_*env vars, using early returns (let-else) instead of nestedif letchains.hoist_conclusion_job_results— hoists upstream job$[dependencies...]results into job-levelJobVariables and wires the corresponding$(name)macro env vars onto the Conclusion step, returning(Vec<JobVariable>, BashStep).build_conclusion_jobnow calls these helpers in a loop / single call instead of inlining ~150 lines of logic.Before / after
build_conclusion_jobflagged at 195/100 lines byclippy::too_many_lines.too_many_linesclippy report for this file at all.Verification
cargo test --bin ado-aw— all 3229 tests pass (1 ignored, unrelated to this change).cargo clippy --all-targets --all-features— clean except one pre-existing, unrelated warning increate_work_item.rs.Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
spsprodeus21.vssps.visualstudio.comTo allow these domains, add them to the
network.allowedlist in your workflow frontmatter:See Network Configuration for more information.