feat(ir): validate front-matter task steps against typed builders

[jamesadevine]

Summary

Adds advisory validation of authored front-matter task steps, built on the typed task builders in src/compile/ir/tasks/. The builders are used in reverse as a schema (parse, don't validate): each derives serde::Deserialize keyed on its real ADO input names with deny_unknown_fields, so a clean deserialization is the validation and a serde error is the diagnostic.

The validation is warning-only — it never fails anything and never alters emitted YAML.

Motivation: feedback for self-optimizing agents

This is primarily for agents that self-optimize by extracting repeated actions into reusable ADO task steps. Such an agent needs a tight, structured correctness signal on the steps it synthesises. So the findings are surfaced through the lint channel the agent already consumes — ado-aw lint (--json) and the lint_workflow MCP tool — not as compile-time stderr noise.

How it works

• validate_task_step(&Value) -> Option<Result<(), String>> (in tasks/parse.rs) looks the step's task: up in a VALIDATORS registry. None = unmodeled task / non-task step (passed through untouched); Some(Ok) = valid; Some(Err) = recognized task with invalid inputs. It never returns an unrecoverable error.
• validate_front_matter_task_steps(...) runs it over the four authored step lists (setup / steps / post-steps / teardown), returning structured TaskStepFindings.
• inspect::lint::lint_front_matter_tasks maps each to a task-input-invalid Warning LintFinding with location { job: <list>, step: <task id> }; build_lint merges these with the existing structural rules so ado-aw lint / lint_workflow report them. Warnings do not fail lint (exit 0).

Coverage

All 46 built-in task builders:

• Flat builders validate via validate_by_deserialize::<T> — field renames mirror the exact ADO input keys (into_step() is the source of truth), enum inputs deserialize from their as_ado_str() tokens, bool inputs accept true and "true" (de_opt_bool_flex), and integer-authored string inputs (e.g. retryCount: 3) accept a bare number (de_opt_str_or_int) to match how ADO coerces them.
• Command / mode-dispatch tasks (Docker@2, DotNetCoreCLI@2, NuGetCommand@2, Npm@1, UniversalPackages@1, PowerShell@2, AzurePowerShell@5, PythonScript@0, VSTest@2, GitHubRelease@1, AzureFileCopy@6, PublishBuildArtifacts@1, JavaToolInstaller@0, AzureCLI@2) supply a bespoke validate_inputs that pops the discriminator — defaulting it to the ADO default variant when omitted — and deserializes the rest into the matching variant struct. Explicit per-variant dispatch (not a serde tagged enum) preserves deny_unknown_fields.

Adding a task = derive Deserialize on the builder + one VALIDATORS line.

Why lint, not compile

• Single source of truth for the finding.
• compile also re-runs in-pipeline (the Stage-1 integrity recompile); a stderr warning there is noise the agent never reads.
• The agent's feedback surface is lint / lint_workflow, so consolidating there closes the self-optimization loop.

Avoiding false positives

Because the audience is an automated producer, a false positive is worse than a gap (it trains the agent to ignore warnings). Guards:

• Round-trip tests (assert_roundtrips): into_step() output is fed back through validate_task_step and must re-validate, for all 13 command/mode-dispatch builders plus required-enum flat builders. This catches serde-rename / as_ado_str() drift — the exact class of bug the AzureCLI@2 fix addressed.
• Scalar-tolerant inputs: bool and integer-valued string inputs accept the shapes authors actually write.
• None-means-unvalidated: unmodeled tasks and non-task steps are never flagged, so coverage is strictly additive.

Test plan

• cargo build, cargo clippy --all-targets --all-features, cargo test — all green.
• tasks::parse: valid/invalid cases, integer-authored inputs, null (inputs: ~) dispatch, no-duplicate-task-ids registry check, and the round-trip suite.
• inspect::lint: unit tests for the task-input-invalid rule (warning + location) and the no-false-positive case.
• inspect_integration: ado-aw lint --json on an invalid-input fixture reports task-input-invalid at warning severity and exits 0.
• compiler_tests: the invalid-input fixture compiles successfully, emits the step verbatim, and compile stays silent (validation belongs to lint).

Review feedback addressed

• Integer-authored string inputs (retryCount / delayBetweenRetries / socketTimeout / retryCountOnDownloadFails) no longer false-positive.
• Removed the dead/misleading Deserialize derive from AzureCli / ScriptLocation (validated via the flat AzureCliInputs schema instead).
• Corrected the fixture docstring (compile is silent; findings surface via lint), documented the VALIDATORS O(n) lookup, and added a trailing newline to inspect_integration.rs.

[github-actions]

🔍 Rust PR Review

Summary: Good design and clean implementation overall, but there's one real bug with integer-typed inputs and a silent data-loss risk in the public API surface that should be addressed before callers wire this up.

Findings

🐛 Bugs / Logic Issues

• copy_files.rs — retryCount / delayBetweenRetries reject integer YAML values: These fields are typed Option<String>, but users naturally write retryCount: 3 (an integer literal in YAML). serde_yaml won't coerce an integer to a String, so this produces a false-positive validation error for a perfectly valid step. The bool inputs got de_opt_bool_flex exactly for this reason — the same treatment is needed here. A de_opt_str_or_int helper that accepts String | u64 | i64 and stringifies would fix it, parallel to de_opt_bool_flex.

• parse.rs line 52–65 — condition:, env:, continueOnError:, timeoutInMinutes: are silently dropped: parse_task_step only extracts displayName and inputs; all other ADO step-level keys are discarded. The returned TaskStep will be missing those fields. Since parse_task_step is pub and the doc comment promises a "normalized TaskStep", a future caller wiring this into the compilation path could silently drop user-authored conditions and env vars. Either document the limitation clearly ("does not preserve step-level fields other than displayName") or extract and apply them here.

• parse.rs line 77–80 — misleading error when command: is present but not a string: map.remove("command").and_then(|v| v.as_str()...) returns None for command: 123, so the error fires with "Docker@2 requires a command input" — implying the key is absent, not malformed. A separate bail! for the non-string case would give a clearer message.

✅ What Looks Good

• The three-outcome contract (Ok(Some) / Ok(None) / Err) is the right design: strictly additive, existing workflows never break when new coverage is added.
• Removing command from the map before routing to the per-variant deserializer with deny_unknown_fields is elegant — makes cross-command input contamination structurally impossible.
• de_opt_bool_flex handles the ADO bool-string duality correctly; #[serde(skip)] on display_name is right (it's not an ADO input).
• Error propagation via anyhow .context() throughout, no unwrap()/expect() on user-facing paths.
• Test coverage is solid — roundtrip, missing required, unknown key, bad bool, wrong-command input, missing/unknown command, and passthrough paths are all exercised.

Generated by Rust PR Reviewer for issue #1096 · 85.5 AIC · ⌖ 12.2 AIC · ⊞ 35.3K · ◷

[github-actions]

🔍 Rust PR Review

Summary: Looks good overall — the three-outcome design is clean and the test suite is solid. Two issues worth addressing before merging.

Findings

🐛 Bugs / Logic Issues

• src/compile/ir/tasks/parse.rs:88 — parse_docker silently conflates "command input missing" with "command input has wrong type". When command: 42 is authored, v.as_str() returns None and the and_then evaluates to None, so the error reads "Docker@2 requires a command input" — misleading because the key is present. Consider checking presence and type separately:

  let raw = map.remove("command")
      .context("Docker@2 requires a `command` input")?;
  let command = raw.as_str()
      .with_context(|| format!("Docker@2: `command` must be a string, got {:?}", raw))?
      .to_string();

⚠️ Suggestions

• src/compile/ir/tasks/parse.rs:44 — parse_task_step is pub and fully functional but not called anywhere outside the test module (the PR description explicitly labels this a PoC). Without an integration point, a reader may wonder whether validation actually runs at compile time. A short // TODO: wire into front-matter steps: passthrough (or a tracking issue reference) would close the gap without extra code.

• src/compile/ir/tasks/parse.rs tests — the Value::Null branch in parse_docker (lines 81–82) is reachable when a user authors inputs: ~ and is untested. A one-line test:

  #[test]
  fn docker_null_inputs_is_rejected() {
      let step = yaml("task: Docker@2\ninputs: ~");
      assert!(parse_task_step(&step).is_err());
  }

✅ What Looks Good

• The three-outcome Ok(Some)/Ok(None)/Err contract is exactly right — additive coverage never silently rejects previously-valid workflows.
• #[serde(deny_unknown_fields)] on every command struct means a typo in an input key (Contnets:, buildcontext:) surfaces as an error rather than being silently ignored.
• de_opt_bool_flex handles both native YAML booleans and ADO's string convention uniformly, with a descriptive rejection for anything else.
• #[serde(skip)] on display_name correctly separates the step-level displayName: (extracted from the outer mapping) from the inputs: block — the post-deserialize with_display_name call is the right idiom here.
• Error messages throughout include enough context (task name, command name, field value) to be actionable without log-diving.

Generated by Rust PR Reviewer for issue #1096 · 112.4 AIC · ⌖ 12.5 AIC · ⊞ 36.3K · ◷

[github-actions]

🔍 Rust PR Review

Summary: Looks good overall — clean "parse, don't validate" design with a few small issues worth fixing.

Findings

🐛 Bugs / Logic Issues

• tests/fixtures/invalid-task-input-agent.md:13 — The fixture body says "The compiler should emit an advisory warning", but the design is the opposite: compile stays silent and warnings only surface via ado-aw lint. The compiler_tests.rs test even explicitly asserts !stderr.contains("CopyFiles@2") to verify the silence. The fixture docstring contradicts the test and the design intent — it will confuse the next person who reads it.

• tests/inspect_integration.rs (EOF) — File is missing a trailing newline (\ No newline at end of file in the diff).

⚠️ Suggestions

• src/compile/ir/tasks/azure_cli.rs — dead/misleading Deserialize on ScriptLocation — ScriptLocation now derives Deserialize with #[serde(rename = "inlineScript")] / #[serde(rename = "scriptPath")] on its data variants. The resulting externally-tagged serde format ({"inlineScript": "..."}) does not match the flat ADO format, and the impl is never used by the validation path (which correctly uses the bespoke AzureCliInputs struct with ScriptLocationKind). The derive is dead code, but its presence makes the type look usable for round-trip validation when it isn't. Either remove the Deserialize derive or add a doc-comment explaining why it exists / how it differs from the ADO flat format.

• src/compile/ir/tasks/parse.rs:VALIDATORS — VALIDATORS.iter().find() is O(n) at each call site (once per authored step). Fine at 46 entries, but a sorted array + binary search (trivial with partition_point) or a OnceLock<HashMap<&str, ...>> would future-proof the registry as coverage grows. Not urgent, but worth a brief // TODO: switch to binary search when > ~100 entries if not doing it now.

✅ What Looks Good

• "Parse, don't validate" design is idiomatic — the builder structs doubling as serde schemas with deny_unknown_fields is exactly the right Rust pattern; required-field detection and unknown-field rejection fall out for free.
• Round-trip tests (assert_roundtrips) are excellent — they guard against serde rename tokens and as_ado_str() values drifting out of sync with into_step(), catching exactly the class of bug the AzureCLI fix addressed.
• None-means-unvalidated semantics — cannot produce false positives; coverage is purely additive. Safe to merge incrementally.
• de_opt_bool_flex correctly handles ADO's mixed true / "true" convention across all bool inputs.
• Lint-not-compile surfacing — advisory findings belong in lint, not in the integrity-recompile path. Architectural decision is sound.
• No unwrap() or expect() on user-facing paths in the new code.

Generated by Rust PR Reviewer for issue #1096 · 487.3 AIC · ⌖ 12.8 AIC · ⊞ 34.8K · ◷

[jamesadevine]

Addressed the automated Rust PR Review feedback (latest reviews) in 52d3783:

Bugs fixed

• Integer-authored string inputs (retryCount, delayBetweenRetries, socketTimeout, retryCountOnDownloadFails): authors write these as bare integers (retryCount: 3), which serde rejected against Option<String> → false-positive finding. Added de_opt_str_or_int (accepts a YAML number and stringifies, mirroring ADO coercion) and applied it in copy_files/download_secure_file/use_node. Added a regression test.
• azure_cli.rs dead/misleading Deserialize: dropped the unused derive from AzureCli and ScriptLocation (the externally-tagged enum form never matched the flat ADO shape; AzureCLI@2 validates via the bespoke AzureCliInputs schema). Documented ScriptLocation as construction-only.
• Fixture docstring (invalid-task-input-agent.md): corrected to state compile stays silent and findings surface via ado-aw lint, matching the test.
• Trailing newline added to tests/inspect_integration.rs.

Suggestions applied

• VALIDATORS: documented the O(n) lookup and when to switch to binary search.
• Added a test for the Docker@2 inputs: ~ (null) branch (now defaults gracefully to buildAndPush).

Obsolete points (targeted the pre-rewrite parse_task_step/parse_docker): the function no longer returns a normalized TaskStep (so no step-level fields can be dropped), and it is now wired into the lint channel — so the "silently drops condition/env", "not called anywhere (PoC)", and command-not-a-string error-message findings no longer apply. Command-dispatch tasks also now default an omitted discriminator rather than erroring.

cargo build, cargo clippy --all-targets --all-features, and cargo test are all green.

[github-actions]

🔍 Rust PR Review

Summary: Solid implementation with good test coverage — one case-sensitivity false-positive risk and one maintenance hazard in the two-pass validators worth fixing before merge.

---

Findings

⚠️ Suggestions

src/compile/ir/tasks/common.rs — de_opt_bool_flex silently rejects mixed-case string booleans

de_opt_bool_flex only accepts "true" / "false" (exact lowercase) as string booleans:

"true"  => Ok(Some(true)),
"false" => Ok(Some(false)),
other   => Err(...)   // ← "True", "False", "TRUE" all hit this arm

serde_yaml 0.9+ implements YAML 1.2, where only lowercase true/false are booleans; everything else (True, YES, "True") is deserialized as a String. So an authored CleanTargetFolder: True (capital-T, valid in many YAML editors) arrives here as BoolOrStr::Str("True") and returns an error — a false-positive warning for a step that ADO would accept.

Suggest adding case-insensitive matching, or at minimum accepting "True" / "False":

Some(BoolOrStr::Str(s)) => match s.to_ascii_lowercase().as_str() {
    "true"  => Ok(Some(true)),
    "false" => Ok(Some(false)),
    _       => Err(...)
},

Since all of this is advisory-only, the impact is a spurious warning rather than a broken compile, but it could confuse users/agents whose YAML tools prefer capitalized scalars.

---

src/compile/ir/tasks/vstest.rs and publish_build_artifacts.rs — dual-list maintenance hazard

Both VSTest@2 and PublishBuildArtifacts@1 use a two-pass validation pattern: validate common inputs, then remove them from the map, then dispatch on the variant-specific struct (which has deny_unknown_fields). The "remove" step is a hand-maintained key list that must stay in sync with the struct definition:

// vstest.rs — struct field and removal list are parallel, must stay synchronized
struct VsTestCommonInputs { ... }          // ← source of truth
fn remove_common_inputs(map: ...) { ... }  // ← must mirror it exactly

If a new common field is added to the struct but forgotten in remove_common_inputs, it survives into the variant dispatch and gets rejected as "unknown field" — a false-positive warning. Currently both lists are in sync, but this is an easy mistake as the task schemas evolve.

One low-friction fix is to drive removal from the struct's serde field names via a const slice (same source of truth as the struct). Alternatively, document the invariant clearly and add a test that adds a common input set and verifies it doesn't generate a false-positive across all three selectors.

---

✅ What Looks Good

• Advisory guarantee is airtight: validate_task_step and validate_front_matter_task_steps can never propagate an anyhow::Err or panic. Every code path in parse.rs returns an Option<Result<(), String>>, so the compiler is provably unaffected.
• Round-trip tests: The assert_roundtrips helper + 13 dispatch-builder round-trips is a strong correctness guarantee — verifying that into_step() emits exactly what the Deserialize side accepts eliminates the biggest class of rename/token bugs.
• registry_has_no_duplicate_task_ids test: Simple and valuable — protects against accidentally registering the same task ID twice with different validators.
• de_opt_str_or_int: The integer→string coercion helper is well-scoped and tested (copy_files_integer_authored_retry_count_is_ok). Handles the common author pattern without widening the type unnecessarily.
• Lint integration: Merging structural and task-input findings in build_lint before calling report_from_findings (rather than having two report calls) keeps the JSON output in one sorted list.
• report() → report_from_findings() rename is complete: No orphaned callers found in src/ or src/inspect/.
• FrontMatter not imported into parse.rs: Passing the four step slices individually rather than &FrontMatter keeps the parsing module independent of the front-matter grammar — the right call.

Generated by Rust PR Reviewer for issue #1096 · 365.7 AIC · ⌖ 12.7 AIC · ⊞ 36.4K · ◷

[jamesadevine]

Addressed the latest Rust PR Review suggestions in 3819f4b:

• de_opt_bool_flex case-sensitivity — now matches string booleans case-insensitively (True / False / TRUE). serde_yaml (YAML 1.2) parses capitalized scalars as strings rather than bools, so CleanTargetFolder: True hit the string arm and was wrongly rejected; added a copy_files_capitalized_bool_is_ok test.
• Dual-list maintenance hazard (vstest.rs / publish_build_artifacts.rs) — added drift-guard tests that set every common input and assert validation passes across all selectors / locations. If a common field is added to the *CommonInputs struct but forgotten in remove_common_inputs, the leftover key trips the variant's deny_unknown_fields and the test fails — directly detecting the drift.
• While there, applied de_opt_str_or_int to PublishBuildArtifacts@1 ParallelCount (integer-authored numeric string input), consistent with the earlier retryCount fix.

cargo build, cargo clippy --all-targets --all-features, and cargo test are all green.

[github-actions]

🔍 Rust PR Review

Summary: Well-designed feature with one moderate concern and a couple of minor nits — no blocking issues.

---

Findings

⚠️ Suggestions

• src/inspect/lint.rs:348-360 — LintLocation::job semantic overloading

  For task-input-invalid findings, the job field is set to the step-list name ("setup", "steps", "post-steps", "teardown") rather than an actual ADO job ID. Every other lint finding produced by location_for sets stage: Some(stage_id) and job: Some(actual_job_id). An agent consuming the JSON output and trying to correlate findings with the pipeline graph will see stage: null, job: "steps" for these findings and stage: "Stage1", job: "Agent" for structural findings — a mix that could cause attribute-lookup failures.

  Since the code: "task-input-invalid" already identifies this class, one option would be to use stage: Some("front-matter") as a stable sentinel and keep job for the list name, or to promote a dedicated list field in LintLocation. The current encoding works fine for an agent that branches on code, but it's worth documenting the convention explicitly so future consumers don't have to discover it through testing.

• src/compile/ir/tasks/common.rs:87-95 — de_opt_str_or_int accepts f64

  retryCount: 3.14 deserializes to Some("3.14") (or similar), which passes the validator but ADO would reject at runtime. This is a false-negative, not a false-positive, so it's consistent with the PR's explicit stance of preferring gaps over noise. Worth a brief comment acknowledging the edge case (authors writing bare integer inputs as floats is already unusual; non-integer floats even more so).

• src/compile/ir/tasks/parse.rs:183-187 — redundant unwrap_or_default() on a path already known-non-empty

  Inside the if let Some(Err(...)) = validate_task_step(step) branch, the task string is re-extracted with .unwrap_or_default(). Because validate_task_step itself early-returns None if map.get("task").and_then(Value::as_str) fails, any Some(Err(...)) result guarantees the task: key is present. The unwrap_or_default() is defensive dead code — no bug, but a comment noting why it can't fail would help future readers.

---

✅ What Looks Good

• Round-trip test suite (assert_roundtrips) is the right call. Feeding into_step() output back through the validator is the only way to catch serde(rename) / as_ado_str() drift, and covering all 13 command/mode-dispatch builders plus representative flat-enum builders is thorough.

• None-means-unvalidated contract is cleanly enforced — no validator returns a hard error, and unmapped tasks/non-task steps produce no finding. Coverage is provably additive.

• registry_has_no_duplicate_task_ids test catches a subtle registration mistake before it silently ignores the second entry (since .find() returns the first match). Good defensive test.

• de_opt_bool_flex correctly handles YAML 1.2's true/false-only native boolean parsing by accepting the string form case-insensitively, matching ADO's own coercion.

• serde(skip) + deny_unknown_fields combination is safe here — all skipped fields (display_name, timeout_minutes, etc.) are builder-internal with no corresponding ADO input key, so no false-positive risk on the deny_unknown_fields path.

• Surfacing findings through lint (not compile) is the correct architectural choice: it keeps the in-pipeline integrity recompile quiet and puts feedback exactly where the self-optimizing agent reads it.

Generated by Rust PR Reviewer for issue #1096 · 706.8 AIC · ⌖ 12.2 AIC · ⊞ 36.6K · ◷

[jamesadevine]

Latest Rust PR Review (no blocking issues) addressed in 0246921 — all three were documentation clarifications, applied as such (no behaviour change):

• LintLocation::job overloading — documented the convention on lint_front_matter_tasks: for task-input-invalid findings, stage is None, job is the front-matter step-list name, and step is the task id. Consumers correlating with the graph should branch on code.
• de_opt_str_or_int accepts f64 — added a comment noting this is a deliberate false-negative (e.g. retryCount: 3.14), consistent with the PR's stance of preferring gaps over noise.
• Redundant unwrap_or_default() in validate_front_matter_task_steps — commented why it can't fail (validate_task_step early-returns None unless a string task: key is present).

Separately, added lint guidance to prompts/create-ado-agentic-workflow.md: since the compiler passes raw task: steps through unchecked and task-input validation is lint-only, the prompt now instructs agents to run ado-aw lint <file> --json (or the lint_workflow MCP tool) and fix task-input-invalid findings — reading the JSON findings rather than the always-0 exit code.

cargo build, cargo clippy --all-targets --all-features, and cargo test are green.

[github-actions]

🔍 Rust PR Review

Summary: Looks good — well-structured approach with one concrete coverage gap and two minor design observations worth addressing.

---

Findings

⚠️ Suggestions

src/compile/ir/tasks/parse.rs:608 — Missing round-trip test for AzurePowerShell::file()

roundtrip_azure_powershell_inline_dispatch exercises inline mode only. The PR's stated goal is to round-trip test all command/mode-dispatch builders to catch #[serde(rename)] ↔ as_ado_str() / into_step() drift, but AzurePowerShell::file() → AzurePowerShellFile (the FilePath path) has no corresponding test. ScriptType is the discriminator popped by validate_inputs, so the fields at risk are ScriptPath / ScriptArguments, which differ from the Inline variant and could silently drift. Adding roundtrip_azure_powershell_file_dispatch completes the coverage claim.

---

src/compile/ir/tasks/azure_powershell.rs:86 + into_step():216-220 — Preferred(String::new()) sentinel + trailing push_opt ordering

The custom Deserialize impl creates Preferred("") when it sees azurePowerShellVersion: OtherVersion, relying on the trailing push_opt(&mut t, "preferredAzurePowerShellVersion", self.preferred_azure_powershell_version) to overwrite the empty string when into_step() is called on a deserialized struct. This works correctly because into_step() is never called on deserialized structs in practice, and in the builder path preferred_azure_powershell_version is always None. However, the load-bearing ordering between the match-arm write and the push_opt is easy to break: if a setter for preferred_azure_powershell_version is ever added, the Preferred(ver) match arm's write is silently overwritten. A short comment on the push_opt line clarifying the invariant ("only takes effect in the deserialization path where the sentinel is "" ") would help future maintainers.

---

src/compile/ir/tasks/vstest.rs:65-80 — VsTestCommonInputs missing #[serde(deny_unknown_fields)]

validate_common runs VsTestCommonInputs deserialization to type-check the shared inputs. Because VsTestCommonInputs lacks deny_unknown_fields, a typo in a common field name (e.g. runInParalel) is not caught there — it survives step 1, remains in the map after remove_common_inputs, and surfaces as an unknown field error attributed to the selector-specific struct. The error message then reads testSelector 'testAssemblies': unknown field 'runInParalel' instead of common inputs: .... Not a correctness bug, but the diagnostic is confusing. Adding #[serde(deny_unknown_fields)] to VsTestCommonInputs (and removing selector-specific field names from it) would give accurate attribution.

---

✅ What Looks Good

• "Parse, don't validate" is the right call. Reusing the builder structs as serde schemas means there's a single source of truth for field names; the round-trip tests guard against into_step() ↔ Deserialize drift exactly as intended.
• de_opt_bool_flex and de_opt_str_or_int cleanly handle the YAML 1.2 / ADO coercion mismatch (capitalised True/False, bare integer retry counts) with deliberate false-negative tradeoffs for the edge cases — the comments are clear about this.
• Tristate return from validate_task_step (None / Some(Ok) / Some(Err)) is the right interface for partial coverage: unmodeled tasks pass through silently and the coverage is strictly additive.
• Advisory-only, lint-channel only is the correct placement. The compile path running in-pipeline for integrity must stay silent; consolidating findings in lint / lint_workflow closes the self-optimisation loop cleanly.
• registry_has_no_duplicate_task_ids test is a nice guard against copy-paste errors in the VALIDATORS table.
• #[serde(skip)] on display_name fields correctly prevents the builder-internal field from being treated as an unknown ADO input under deny_unknown_fields.

Generated by Rust PR Reviewer for issue #1096 · 616 AIC · ⌖ 12.4 AIC · ⊞ 35K · ◷

[jamesadevine]

Latest review addressed in 07b8dcc:

• Missing AzurePowerShell::file() round-trip — added roundtrip_azure_powershell_file_dispatch, completing the all-dispatch-builders coverage (the FilePath path's ScriptPath/ScriptArguments were previously unguarded against rename/into_step drift).
• Preferred("") sentinel — documented the invariant at its source (the Deserialize impl): the empty string is a placeholder because the pinned version lives in a separate preferredAzurePowerShellVersion input, and into_step() is never called on a deserialized value, so it never reaches emitted YAML.
• VsTestCommonInputs deny_unknown_fields — I did not add it: validate_common runs on the full input map, which still contains the selector-specific inputs (testAssemblyVer2, testPlan, …), so denying unknowns there would reject those valid fields (the drift-guard test confirms this). The struct holds only common fields, so there are no selector fields to remove. Typos are still caught and named by the selector variant's own deny_unknown_fields; I documented this design on validate_common.

cargo build, cargo clippy --all-targets --all-features, and cargo test are green.

[github-actions]

🔍 Rust PR Review

Summary: Solid implementation — the parse-don't-validate approach is idiomatic and the round-trip test suite is an excellent guard against the primary failure mode. Two design-level concerns worth noting; no outright bugs found.

Findings

⚠️ Suggestions

• src/inspect/lint.rs:355-364 — LintLocation.job semantic overload for task-input-invalid

  The finding encodes job: Some("steps") (a step-list name) where all other finding types encode a real job id. Agents consuming the JSON lint output that correlate location.job with a pipeline job by identity — not branching on code == "task-input-invalid" first — will silently misinterpret the field. The doc comment correctly documents the convention, but a dedicated field (e.g., list) on LintLocation would make the distinction machine-readable rather than only prose-readable. As-is, any consumer that doesn't read the comment has no structural signal to distinguish the two uses.

• src/inspect/lint.rs:358 — step index not addressable via location for duplicate task instances

  When the same task type appears twice in a step list with separate errors (e.g., two CopyFiles@2 steps each with different invalid inputs), both findings land at location = { job: "steps", step: "CopyFiles@2" }. The index only appears inside the message string as (in \steps[0]`). A structured consumer (agent or tool) that wants to programmatically identify and fix the nth instance must parse the message, which is fragile. Including the index in a structured field — even as part of step(e.g.,"CopyFiles@2[0]"`) — would make addressing unambiguous.

• src/compile/ir/tasks/vstest.rs:40 / publish_build_artifacts.rs:51 — double-clone in validate_common

  validate_common(&Value::Mapping(map.clone()))  // clone 1
  // inside validate_common:
  serde_yaml::from_value::<T>(inputs.clone())    // clone 2

  validate_common takes &Value and then must clone again because serde_yaml::from_value consumes its argument. Changing the signature to take Value and calling validate_common(Value::Mapping(map.clone())) halves the allocations. Minor for lint workloads but the pattern repeats across several task files.

✅ What Looks Good

• Round-trip tests (assert_roundtrips) are the right kind of test here — they guard the exact class of bug (serde rename / as_ado_str() token drift) that was previously identified and fixed. Covering all 13 command-dispatch builders plus required-enum flat builders is comprehensive.
• None-means-unvalidated default is correct: coverage is strictly additive and existing workflows can never regress.
• de_opt_bool_flex / de_opt_str_or_int correctly bridge the authoring style vs. ADO's type model. The deliberate float false-negative is well-documented in common.rs.
• VsTestCommonInputs without deny_unknown_fields (then selector variants with it) is a thoughtful split: common-input typos fall through to the selector-specific struct where the field name is attributed with good context.
• registry_has_no_duplicate_task_ids test is a clean defensive check for VALIDATORS maintenance drift.
• Warning-only + exit 0 + compile silence cleanly separates the agent feedback channel from the compile path.

Generated by Rust PR Reviewer for issue #1096 · 564.7 AIC · ⌖ 12.6 AIC · ⊞ 36.6K · ◷