feat(compile): add Conclusion job for pipeline failure reporting

[jamesadevine]

Summary

Adds an always-running Conclusion job to the canonical pipeline shape, mirroring gh-aw's conclusion job pattern. The Conclusion job reports pipeline failures and diagnostic signals (noop, missing-tool, missing-data) to Azure DevOps work items.

Pipeline shape

Setup → Agent → Detection → SafeOutputs → Teardown → Conclusion
                                                        ↑
                                              condition: always()

The Conclusion job is always emitted when safe-outputs: exists (gh-aw pattern — noop is always-on).

Configuration (under safe-outputs:)

Three opt-out levels:

# Global opt-out — no failure work items at all
safe-outputs:
  report-failure-as-work-item: false

# Per-tool opt-out
safe-outputs:
  noop:
    report-as-work-item: false
  missing-tool:
    report-as-work-item: false

# Disable tool entirely (agent can't call it)
safe-outputs:
  noop: false

Per-tool config (aligned with gh-aw's flat field style):

safe-outputs:
  noop:
    title-prefix: "[ado-aw] Agent reported no operation"
    work-item-type: Task
    area-path: "MyProject\\MyTeam"
    tags:
      - agent-noop
  missing-tool:
    title-prefix: "[ado-aw] Missing tool"
    work-item-type: Bug
    report-as-work-item: true

Key changes

Area	What
ado-script bundle	scripts/ado-script/src/conclusion/index.ts → conclusion.js (ncc-bundled)
Pipeline compilation	build_conclusion_job() in agentic_pipeline.rs, emitted when safe_outputs is non-empty
Config surface	Lives entirely under safe-outputs: — no separate conclusion: block
Codemod	0003_flatten_work_item_config migrates old noop: work-item: {title, ...} → flat noop: {title-prefix, ...}
WI write helpers	createWorkItem, addWorkItemComment, findWorkItemByTitle, fileOrAppendWorkItem in shared/wit.ts
Separation of concerns	Work-item filing removed from noop.rs/missing_tool.rs (now handled by conclusion job)
Module rename	src/safeoutputs/ → src/safe_outputs/ (snake_case consistency)

How it works

1. Compiler emits a Conclusion job whenever safe-outputs: is configured
2. The job downloads the safe_outputs artifact (contains safe-outputs-executed.ndjson)
3. conclusion.js reads upstream job results + the NDJSON manifest + per-tool config (passed as AW_NOOP_CONFIG etc. JSON env vars)
4. Files/comments ADO work items for: pipeline failures, noop, missing-tool, missing-data
5. Uses WIQL dedup: searches for existing open WI with same title → appends comment; else creates new
6. Never fails the pipeline — filing errors are logged as warnings

Test plan

• cargo build ✅
• cargo test conclusion — 4 Rust tests pass (1 types + 3 compiler integration)
• cargo test flatten_work_item — 6 codemod tests pass
• npx vitest run src/conclusion — 8 TypeScript tests pass (conclusion.js logic)
• npx vitest run src/shared/__tests__/wit.test.ts — WI write helper tests pass
• npm run typecheck — TypeScript type-checks clean

[github-actions]

🔍 Rust PR Review

Summary: Generally solid — the Conclusion job wires up correctly and the TypeScript logic handles edge cases well. Two issues worth fixing before merge.

Findings

🐛 Bugs / Logic Issues

• [src/compile/agentic_pipeline.rs, build_conclusion_job] serde_json::to_string().unwrap_or_default() silently swallows serialization errors, violating the project's anyhow propagation convention. While Vec<String> serialization won't practically fail, if it did you'd silently emit AW_WORK_ITEM_TAGS="" and tags would be dropped with no error in the build log. Should be:

  let tags_json = serde_json::to_string(&conclusion_config.tags)
      .context("failed to serialize conclusion tags")?;

• [src/compile/agentic_pipeline.rs, build_conclusion_job] The DownloadPipelineArtifact@2 step condition is SucceededOrFailed, but when the Agent job fails early (before SafeOutputs publishes the artifact), the download step will turn red in the pipeline UI — even though conclusion.js handles the missing manifest gracefully via its own Always condition. This creates confusing noise. Consider condition: always() + continueOnError: true on the download step so operators see the intended "artifact unavailable, continuing" signal rather than a red failure.

⚠️ Suggestions

• Silent breaking change for safe-outputs: noop: work-item: / safe-outputs: missing-tool: work-item: configs. The WorkItemReportConfig, NoopConfig, and MissingToolConfig structs were removed. Since FrontMatter.safe_outputs is an opaque HashMap<String, serde_json::Value>, existing pipelines with these sub-keys will still parse but silently lose their WI-filing behavior — no compile error, no warning. This is a real behavior regression for any consumer who configured these. The test fixtures do show the migration (work-item: enabled: false → noop: {}), but there's no codemod registered in src/compile/codemods/ to automate it. Given the project's codemod framework (docs/codemods.md), adding one here would be the right call.

✅ What Looks Good

• WIQL injection in wit.ts findWorkItemByTitle is correctly handled: title.replaceAll("'", "''") is the proper WIQL string-literal escape, and the title originates from sanitized operator-controlled front matter (not agent-supplied content).
• Artifact name consistency: safe_outputs artifact is published by the SafeOutputs job and downloaded by the same name in the Conclusion job — consistent throughout.
• EXECUTED_NDJSON_FILENAME = "safe-outputs-executed.ndjson" matches the TypeScript EXECUTED_MANIFEST_FILENAME constant and the Rust ndjson.rs export — no mismatch.
• fileSignal error handling: errors are caught as warnings rather than re-thrown, ensuring the Conclusion step never itself fails the pipeline due to a WI API error.
• wire_explicit_dependencies correctly gates the Teardown dependency on has_teardown, so pipelines without a Teardown job get dependsOn: [Agent, Detection, SafeOutputs] only.
• always() on both job and conclusion.js step correctly ensures the reporter runs even when all upstream jobs failed or were cancelled.
• ConclusionConfig uses deny_unknown_fields — typos in conclusion: front matter will fail fast at parse time.
• Good test coverage: 3 front-matter unit tests in types.rs + 3 compiler integration tests in compiler_tests.rs + TypeScript vitest suite.

Generated by Rust PR Reviewer for issue #1076 · 1K AIC · ⌖ 12.5 AIC · ⊞ 35.4K · ◷

[github-actions]

🔍 Rust PR Review

Summary: Needs changes — two correctness bugs that will silently break the core feature (failure detection and per-tool opt-out), plus a data-loss codemod bug.

---

Findings

🐛 Bugs / Logic Issues

1. $[dependencies.*.result] will not expand in step env: blocks (src/compile/agentic_pipeline.rs, lines ~1162–1173)

let agent_result = format!("$[dependencies.{}.result]", agent_id.as_str());
// ...
conclusion_step = conclusion_step
    .with_env("AW_AGENT_RESULT", EnvValue::Literal(agent_result))
    .with_env("AW_DETECTION_RESULT", EnvValue::Literal(detection_result))
    .with_env("AW_SAFEOUTPUTS_RESULT", EnvValue::Literal(safeoutputs_result));

ADO only evaluates $[...] runtime expressions inside job-level variables: definitions and condition: fields — not in step env: blocks. These three env vars will receive the literal strings $[dependencies.Agent.result] etc. at runtime, so conclusion.js will see "$[dependencies.Agent.result]" instead of "Succeeded" or "Failed", and its failure-detection logic will never trigger.

The fix is to hoist these to job-level variables: on the Conclusion job and reference them as $(AW_AGENT_RESULT) in the step env. Notably, the existing test comment in compiler_tests.rs at the bottom of the file documents exactly this pitfall ("ADO ONLY evaluates $[ ... ] inside variables: mappings and condition: fields").

---

2. Per-tool configs are emitted by Rust but never consumed by conclusion.js (src/compile/agentic_pipeline.rs vs scripts/ado-script/src/conclusion/index.ts)

The Rust compiler passes AW_NOOP_CONFIG, AW_MISSING_TOOL_CONFIG, AW_MISSING_DATA_CONFIG as JSON env vars:

for tool_key in &["noop", "missing-tool", "missing-data"] {
    if let Some(tool_config) = front_matter.safe_outputs.get(*tool_key) {
        let env_key = format!("AW_{}_CONFIG", tool_key.to_uppercase().replace('-', "_"));
        // ...
    }
}

But loadConfig() in conclusion.js reads only global flat vars (AW_WORK_ITEM_TYPE, AW_WORK_ITEM_AREA_PATH, etc.) and never reads AW_NOOP_CONFIG et al. Consequences:

• noop.title-prefix, noop.work-item-type, noop.area-path, noop.tags — all silently ignored.
• Per-tool opt-out (noop.report-as-work-item: false) does not work despite being documented in the PR and in docs/conclusion.md.
• The only opt-out that actually functions is the global report-failure-as-work-item: false.

The conclusion test (test_conclusion_job_emits_expected_env_vars_for_conclusion_script) verifies that AW_NOOP_CONFIG is emitted with the right JSON, but there is no test verifying that conclusion.js actually reads and applies it.

---

3. Codemod silently drops non-mapping work-item: values (src/compile/codemods/0003_flatten_work_item_config.rs, ~line 100)

let Value::Mapping(wi_map) = wi_val else {
    // work-item was present but not a mapping — put it back as-is
    continue;   // <── wi_val was already removed; this drops it
};

tool_map.remove(&wi_key) is called unconditionally before this let-else. If work-item: is present but isn't a Mapping (e.g., work-item: true), the value is removed and never re-inserted — silent data loss. The comment acknowledges the intent to restore it, but the code doesn't. Fix:

let Value::Mapping(wi_map) = wi_val else {
    tool_map.insert(wi_key, wi_val); // restore
    continue;
};

---

4. Default work-item type is "Bug" in TypeScript but "Task" everywhere else (scripts/ado-script/src/conclusion/index.ts, loadConfig)

workItemType: readOptionalEnv(env, "AW_WORK_ITEM_TYPE") ?? "Bug",

AW_WORK_ITEM_TYPE is never set by the Rust compiler (no global work-item-type env var is emitted for the conclusion step). The PR description and old WorkItemReportConfig default both say "Task". All conclusion work items will be filed as Bugs. This should be "Task" to match the documented default, or the Rust compiler should emit AW_WORK_ITEM_TYPE from the front matter.

---

✅ What Looks Good

• The architectural separation of concerns is clean: noop/missing-tool are now pure pass-through recorders, and the Conclusion job owns all WI filing. This is a good design.
• The wire_explicit_dependencies update (adding Teardown-conditional dep) is correct — condition: always() on the Conclusion job ensures it runs even when dependencies are skipped.
• WorkItemReportConfig duplication between Rust and TypeScript is reduced; the TypeScript wit.ts helpers look well-structured.
• Codemod 0003 correctly detects conflicts between flat fields and nested work-item: and bails with an actionable error.
• The WIQL single-quote escaping (replaceAll("'", "''")) is the correct convention for WIQL string literals.

Generated by Rust PR Reviewer for issue #1076 · 197.3 AIC · ⌖ 13.2 AIC · ⊞ 35.4K · ◷

[github-actions]

🔍 Rust PR Review

Summary: Solid architecture with one wiring gap and a few logic nuances worth addressing before merge.

---

Findings

🐛 Bugs / Logic Issues

conclusion.js reads global WI config env vars that build_conclusion_job never emits (src/compile/agentic_pipeline.rs, scripts/ado-script/src/conclusion/index.ts)

loadConfig() consumes AW_WORK_ITEM_TYPE, AW_WORK_ITEM_AREA_PATH, AW_WORK_ITEM_ITERATION_PATH, AW_WORK_ITEM_TAGS, AW_WORK_ITEM_TITLE, and AW_INCLUDE_STATS from the environment. build_conclusion_job never emits any of them. These serve as global fallbacks in buildWorkItemConfig — most visibly for the pipeline_failure signal, which has no per-tool config entry and therefore always gets workItemType: "Task", empty area/iteration paths, and no tags.

Users can configure per-tool settings via noop.area-path: etc., but there is currently no front-matter path to control the global pipeline-failure WI type, area, iteration path, tags, or title template. Either:

• Expose the missing fields in the safe-outputs: front-matter block and emit the corresponding env vars in build_conclusion_job, or
• Document the current limitation explicitly in docs/conclusion.md so users aren't surprised.

---

report-failure-as-work-item: false disables all WI filing, not just pipeline failures (scripts/ado-script/src/conclusion/index.ts:main)

if (!config.reportFailureAsWorkItem) {
  logInfo("Conclusion work-item filing disabled via AW_REPORT_FAILURE_AS_WORK_ITEM=false");
  return 0;  // exits before filing noop/missing-tool/missing-data WIs too
}

The name and the PR description ("Global opt-out — no failure work items at all") suggest this flag is intentionally global. But buildWorkItemConfig also redundantly uses it as the enabled field passed to fileOrAppendWorkItem:

return {
  enabled: config.reportFailureAsWorkItem,  // always true here because of the early return above
  ...
};

This is dead code — enabled can never be false at that callsite. Low severity, but worth cleaning up to avoid confusion in future maintenance.

---

No per-tool opt-out path for pipeline_failure signals (scripts/ado-script/src/conclusion/index.ts)

getToolConfigKey("pipeline_failure") returns "pipeline_failure", but config.toolConfigs only contains noop, missing_tool, and missing_data. So toolConfig is always undefined for pipeline-failure events, meaning:

• The per-tool opt-out check in fileSignal (if (toolConfig && !toolConfig.reportAsWorkItem)) is silently skipped.
• The only way to disable pipeline-failure WI filing is the global flag — which disables all WI filing.

This should be documented; it's a footgun for users who want "no pipeline-failure WIs but still want noop WIs."

---

⚠️ Suggestions

grep "ado-script.zip" checksums.txt | sha256sum -c - is too broad (src/compile/agentic_pipeline.rs download_script)

A partial match on "ado-script.zip" will also hit lines like ado-script.zip.sig. Use an anchored pattern to avoid accidental mismatch:

grep -E '\bado-script\.zip$' checksums.txt | sha256sum -c -

---

Conclusion job unconditionally runs checkout_self_step()

The Conclusion job only runs conclusion.js (downloaded at runtime). A self-checkout adds latency for no purpose unless there's an implicit pool requirement driving it.

---

No integration test for the Teardown + Conclusion coexistence path

wire_explicit_dependencies conditionally adds Teardown to Conclusion's dependsOn when has_teardown, but there's no fixture that exercises both. Worth adding a conclusion_with_teardown.md fixture to prevent future regressions on that branch.

---

✅ What Looks Good

• WIQL escaping in findWorkItemByTitle — replaceAll("'", "''") is the correct SQL-style WIQL escaping; @project binding correctly avoids project-name injection.
• ADO runtime expression hoisting — Job-level $[dependencies.{job}.result] variables referenced as $(AW_*_RESULT) macros in the step env is the correct ADO pattern and is well-commented.
• Manifest parsing resilience — readManifestEntries handles missing file, read errors, and malformed lines independently, and DownloadPipelineArtifact@2 is marked continue_on_error: true, so a failed SafeOutputs job won't block the conclusion script.
• Codemod 0003 conflict detection — bail! on overlapping flat keys is the right call; no silent data loss on ambiguous migrations.
• Module rename (safeoutputs → safe_outputs) is a clean consistency win throughout.

Generated by Rust PR Reviewer for issue #1076 · 350.4 AIC · ⌖ 12.8 AIC · ⊞ 33.8K · ◷

[github-actions]

🔍 Rust PR Review

Summary: needs changes — two correctness bugs and a documentation/front-matter mismatch that would produce hard parse errors for users following the docs.

Findings

🐛 Bugs / Logic Issues

EnvValue::AdoMacro("$(SC_WRITE_TOKEN)") — double macro expansion

src/compile/agentic_pipeline.rs — build_conclusion_job:

if has_write_sc {
    conclusion_step = conclusion_step.with_env(
        "SYSTEM_ACCESSTOKEN",
        EnvValue::AdoMacro("$(SC_WRITE_TOKEN)"),
    );
}

lower_env_value lowers AdoMacro(name) as format!("$({name})"), so this emits $($(SC_WRITE_TOKEN)) in the generated YAML — a double-wrapped macro that ADO evaluates as a literal string. The Conclusion job will have the wrong (empty) token whenever a write service-connection is configured, silently breaking work-item filing.

The correct form is EnvValue::secret("SC_WRITE_TOKEN") (or PipelineVar), which lowers to $(SC_WRITE_TOKEN). Note that EnvValue::AdoMacro is also being constructed directly (bypassing the ado_macro() allowlist factory), and "SC_WRITE_TOKEN" is not in ALLOWED_ADO_MACROS anyway — the fallthrough else branch correctly uses EnvValue::secret("System.AccessToken"), so the same pattern should be applied here.

---

docs/conclusion.md describes a conclusion: front-matter block that doesn't exist

docs/conclusion.md (new file) and docs/front-matter.md (+2 lines) both document config under a top-level conclusion: key:

conclusion:
  work-item-type: Bug
  area-path: "MyProject\\MyTeam"

But FrontMatter has #[serde(deny_unknown_fields)] and no conclusion field was added to the struct (the types.rs diff is only a module-path rename + tests). Any user who writes conclusion: in front matter will get a hard parse error from the compiler. The actual implementation reads configuration from safe-outputs: (confirmed by build_conclusion_job's front_matter.safe_outputs.get(...) calls and the test fixture at tests/fixtures/conclusion_basic.md).

The PR description itself says "Lives entirely under safe-outputs: — no separate conclusion: block", so these doc additions appear to be leftover from an earlier design.

---

docs/conclusion.md field names and defaults don't match the implementation

The table in docs/conclusion.md documents:

• work-item-title → but the code reads title-prefix
• Default for work-item-type → "Bug" → but buildWorkItemConfig (TypeScript) defaults to "Task"

The test fixture uses title-prefix and work-item-type: Bug (user-set), so users reading the docs will use wrong field names.

🔒 Security Concerns

ALLOWED_ADO_MACROS allowlist bypass

src/compile/agentic_pipeline.rs — the new code constructs EnvValue::AdoMacro(...) directly rather than calling EnvValue::ado_macro(...). The ado_macro() factory validates the name against ALLOWED_ADO_MACROS; the direct variant constructor skips that check. The rest of the codebase consistently uses the factory to ensure compile-time macro hygiene. (This is a secondary consequence of the double-macro bug above.)

⚠️ Suggestions

• Variable shadowing (src/compile/agentic_pipeline.rs, ~line 1190): inside the per-tool loop, let prefix = format!("AW_{}", ...) shadows the prefix: &JobPrefix<'_> parameter. Rename to env_prefix to avoid confusion and the Rust unused_variable / shadowing warnings.

• Unnecessary checkout step: build_conclusion_job starts with checkout_self_step() but the job only runs a downloaded bundle (conclusion.js) and a pipeline artifact. No repo code is needed; removing the checkout saves ~30s per run.

✅ What Looks Good

• The three-layer opt-out design (global report-failure-as-work-item, per-tool report-as-work-item, noop: false) is clean and correctly wired end-to-end.
• wire_explicit_dependencies correctly adds has_teardown as an optional dep so Conclusion runs after Teardown when present.
• The WIQL single-quote escaping (title.replaceAll("'", "''")) in wit.ts is correct for ADO WIQL injection defence.
• Moving work-item filing out of noop.rs/missing_tool.rs Stage 3 executors and into the dedicated Conclusion job is a sound architectural improvement.
• Codemod 0003_flatten_work_item_config is well-tested (6 cases including conflict detection and idempotency).

Generated by Rust PR Reviewer for issue #1076 · 477.8 AIC · ⌖ 12.7 AIC · ⊞ 33.8K · ◷

[github-actions]

🔍 Rust PR Review

Summary: Looks good overall — clean architecture shift, solid test coverage, and the migration path (codemod + backward-compat) is well-handled. A few issues worth addressing before merge.

Findings

🐛 Bugs / Logic Issues

• conclusion/index.ts:~390 — renderTitle dead code / titlePrefix template substitution silently dropped

  In buildWorkItemConfig, renderTitle is always called with undefined as the template:

  title: toolConfig?.titlePrefix ??
    renderTitle(undefined, config.pipelineName, signal.kind, signal.defaultTitle),

  renderTitle returns undefined when template is undefined, so this always reduces to toolConfig?.titlePrefix ?? undefined. The {pipeline_name} / {signal} substitution logic inside renderTitle is never exercised. Intent unclear: if titlePrefix is meant to support {pipeline_name} placeholders (as renderTitle implies), the call should be renderTitle(toolConfig?.titlePrefix, ...). If not, remove the dead call.

• agentic_pipeline.rs:~1190 — v.to_string() for boolean env var emits JSON-encoded string

  if let Some(v) = obj.get("report-as-work-item") {
      conclusion_step = conclusion_step.with_env(
          format!("{env_prefix}_REPORT_AS_WORK_ITEM"),
          EnvValue::Literal(v.to_string()),
      );
  }

  serde_json::Value::to_string() on Bool(false) gives "false" (correct), but on String("false") gives "\"false\"" (a JSON-encoded string, including the quotes). readBooleanEnv in TypeScript will then log a warning and default to true — silently inverting the intended opt-out. For all other string fields in the same block (title-prefix, area-path, etc.) the code correctly uses .as_str(). This field should too: v.as_bool().map(|b| b.to_string()), and skip the env var if the value isn't a boolean.

⚠️ Suggestions

• agentic_pipeline.rs / conclusion/index.ts — pipeline_failure signal is not configurable via front matter

  The compiler emits AW_NOOP_*, AW_MISSING_TOOL_*, AW_MISSING_DATA_* env vars for per-tool customisation, but no AW_PIPELINE_FAILURE_* vars. loadConfig() in conclusion/index.ts similarly has no pipeline_failure entry in toolConfigs. This means work-item type, area path, and tags for pipeline failure work items are hardcoded (Task, no area/iteration path). Given the PR description explicitly positions this as a per-tool opt-out feature, the omission may be intentional — if so, worth a comment in the code noting it as a deliberate limitation.

• conclusion/index.ts:~470 — enabled field in WorkItemReportConfig is unreachable from conclusion.js

  buildWorkItemConfig sets enabled: config.reportFailureAsWorkItem, but main() already returns early when that flag is false before any call to fileSignal/fileOrAppendWorkItem. The if (!config.enabled) guard inside fileOrAppendWorkItem (in wit.ts) is dead code from conclusion.js's call site. Not a bug, but a misleading contract — the enabled field exists to be driven externally, yet conclusion.js always passes true. Consider omitting enabled from the config passed here or documenting the invariant.

✅ What Looks Good

• WIQL injection protection correctly preserved and ported to TypeScript (title.replaceAll("'", "''"))
• continue_on_error: true on the artifact download step — right call for fault tolerance when SafeOutputs was skipped/failed
• Dependency wiring handles optional Teardown gracefully with the has_teardown guard
• Codemod 0003 is thorough: conflict detection, idempotency test, enabled→report-as-work-item rename all covered
• withRetry wrapper on all ADO SDK calls in wit.ts — good resilience
• Stage 3 pass-through (noop/missing-tool become plain success in execute) is the right model; execute_impl no longer needs credentials it previously degraded without

Generated by Rust PR Reviewer for issue #1076 · 536.6 AIC · ⌖ 12.7 AIC · ⊞ 35.7K · ◷

[github-actions]

🔍 Rust PR Review

Summary: Solid implementation with one real resilience bug worth fixing before merge; everything else is high quality.

---

Findings

🐛 Bugs / Logic Issues

• src/compile/agentic_pipeline.rs — Conclusion job can fail the pipeline on infra errors

  UseNode@1 and the Download ado-script bundle bash step have no condition: always() or continue_on_error: true. Since the job runs with condition: always(), individual steps default to condition: succeeded(). If UseNode@1 fails (unusual but possible), the download step is skipped — but the conclusion.js bash step still executes (it has condition: always()) and runs node .... If the agent pool doesn't already have Node.js on $PATH, that bash step exits 127, the Conclusion job is marked Failed, and the overall pipeline shows as failed even when the agent ran successfully. This directly contradicts the PR description's "Never fails the pipeline" guarantee.

  Minimal fix: add continue_on_error = true to the UseNode@1 TaskStep (same as the artifact-download step), or add a Node fallback guard to the conclusion.js bash step.

• src/compile/agentic_pipeline.rs — Duplicated comment block

  Around the for tool_key in &["noop", "missing-tool", "missing-data"] loop there are two consecutive copies of the same comment:

  // Pass per-tool configs as individual flat env vars (gh-aw pattern).
  // Each field gets its own env var — avoids JSON-in-env-var corruption in ADO.
  // Pass per-tool configs as individual flat env vars (gh-aw pattern).  ← duplicate
  // Each field gets its own env var — avoids JSON-in-env-var corruption in ADO.  ← duplicate
  

  Copy-paste artifact; easy to clean up.

⚠️ Suggestions

• scripts/ado-script/src/conclusion/index.ts — titlePrefix naming is misleading

  PerToolConfig.titlePrefix and the front-matter field title-prefix act as a full title template (with {pipeline_name} / {signal} substitutions that replace the default title entirely), not as a string prepended to something. A user reading title-prefix: "[ado-aw] Noop" would reasonably expect the pipeline name to be appended automatically. Consider renaming to title-template (with a codemod if needed) or at minimum expanding the front-matter docs to clarify this is the complete title.

• scripts/ado-script/src/conclusion/index.ts — pipeline_failure silently falls through toolConfigs

  In loadConfig(), toolConfigs contains keys noop, missing_tool, missing_data — but not pipeline_failure. In fileSignal(), config.toolConfigs[getToolConfigKey("pipeline_failure")] returns undefined, so the per-tool opt-out branch is skipped and filing always proceeds. This is intentional, but it's a gotcha for future contributors. A short comment on the pipeline_failure case in fileSignal() (or an explicit guard) would prevent a future author from being surprised when trying to add per-tool config for failures.

✅ What Looks Good

• Job-variable hoisting for $[dependencies.X.result] — the conclusion_variables approach is exactly right; ADO only evaluates runtime expressions in variables: and condition:, so hoisting to job variables and referencing via $(name) macros in the step env is the correct idiom.
• ir/lower.rs — EnvValue::AdoMacro double-expansion guard — new defensive check that rejects $(Already.Wrapped) names in AdoMacro is a smart addition that prevents silent empty-string expansions in generated YAML.
• WIQL dedup logic in wit.ts — replaceAll("'", "''") is the correct WIQL escaping, and the NOT IN ('Closed', 'Resolved', 'Done') state filter covers all three built-in ADO process templates. Mirrors the deleted Rust implementation faithfully.
• process.exit(0) crash handler — ensures the Conclusion job never hard-fails the pipeline from a TypeScript runtime crash, which is the right default for a best-effort reporting job.
• Codemod 0003_flatten_work_item_config — conflict detection (bail! when flat field and nested work-item: coexist) and the idempotency test are both solid; the transformation is safe to run repeatedly.
• SanitizeContent still applied — noop/missing_tool results are sanitized in Stage 3 before being written to safe-outputs-executed.ndjson, so the Conclusion script reads operator/sanitized content, not raw agent strings.
• Module rename safeoutputs → safe_outputs — clean and the path updates are exhaustive across common.rs, mcp.rs, execute.rs, main.rs, types.rs, etc.

Generated by Rust PR Reviewer for issue #1076 · 318.9 AIC · ⌖ 12.6 AIC · ⊞ 34.1K · ◷

[github-actions]

🔍 Rust PR Review

Summary: Good architectural direction — moving WI filing from Stage 3 into an always-running Conclusion job is the right separation of concerns. One real bug will silently neutralise the manifest-based signal reporting (noop/missing_tool/missing_data) on every pipeline run.

---

Findings

🐛 Bugs / Logic Issues

src/compile/agentic_pipeline.rs — safe-outputs-executed.ndjson never reaches the safe_outputs artifact

The Conclusion job downloads the safe_outputs artifact and reads safe-outputs-executed.ndjson from it:

// build_conclusion_job – download step
.with_input("artifact", "safe_outputs")
.with_input("path", "$(Pipeline.Workspace)/conclusion_inputs");
// ...and the env var:
EnvValue::Literal("$(Pipeline.Workspace)/conclusion_inputs".to_string()),

But the execute_safe_outputs step in build_safeoutputs_job writes the executed NDJSON file to $(Pipeline.Workspace)/analyzed_outputs_$(Build.BuildId)/:

"ado-aw execute ... --safe-output-dir \"$(Pipeline.Workspace)/analyzed_outputs_$(Build.BuildId)\" ..."

The safe_outputs artifact is published from $(Agent.TempDirectory)/staging. The existing copy_logs_safeoutputs_step only copies logs — it never copies safe-outputs-executed.ndjson to staging. The Conclusion job will therefore always log:

Conclusion manifest not found: $(Pipeline.Workspace)/conclusion_inputs/safe-outputs-executed.ndjson

...and skip noop/missing_tool/missing_data signal reporting entirely. Pipeline-failure reporting (driven by upstream job results, not the manifest) is unaffected and will still work.

Fix — add a copy into copy_logs_safeoutputs_step or a dedicated step before the Publish:

cp "$(Pipeline.Workspace)/analyzed_outputs_$(Build.BuildId)/safe-outputs-executed.ndjson" \
   "$(Agent.TempDirectory)/staging/safe-outputs-executed.ndjson" 2>/dev/null || true

---

⚠️ Suggestions

src/compile/agentic_pipeline.rs:~1254 — use inside function body

use crate::compile::ir::job::JobVariable;
let conclusion_variables = vec![...];

use inside a function body is valid Rust but unusual here: all other IR imports are at the top of the file. Moving it alongside use super::ir::job::{Job, Pool} keeps things consistent.

---

✅ What Looks Good

• ADO job-variable hoisting pattern for upstream results is exactly right — $[dependencies.X.result] in a job variable, referenced as $(VAR) in the step env block. Lowering path is correct.
• EnvValue::AdoMacro validation guard in lower.rs is a solid defence-in-depth addition; the double-wrapped $($(...)) case was a real footgun.
• report-as-work-item / serde_json::Value::to_string note (the comment about JSON-encoded strings producing "\"false\"" vs "false") — thorough and prevents a silent opt-out inversion.
• WIQL single-quote escaping in wit.ts::findWorkItemByTitle is sufficient for operator-controlled title inputs; only ' needs doubling in WIQL string literals.
• Checksum verification (sha256sum -c -) in the bundle download step is good supply-chain hygiene.
• Codemod handles conflict detection, idempotency, and all three diagnostic tool keys correctly.
• continue_on_error = true on the DownloadPipelineArtifact step is the right choice — the artifact may not exist when SafeOutputs was skipped.

Generated by Rust PR Reviewer for issue #1076 · 922.5 AIC · ⌖ 12.5 AIC · ⊞ 34.1K · ◷

[github-actions]

🔍 Rust PR Review

Summary: Solid feature with good test coverage — one correctness bug needs fixing before merge.

Findings

🐛 Bugs / Logic Issues

• src/compile/agentic_pipeline.rs ~L1100, conclusion_script — The conclusion bash step has condition: always() but does not check whether conclusion.js was successfully downloaded before calling node. If the "Download ado-script bundle" bash step fails (e.g., network error, wrong version in a release), conclusion.js won't exist at /tmp/ado-aw-scripts/ado-script/conclusion.js. The step then runs node /nonexistent/path, exits non-zero, and the Conclusion job fails — directly contradicting the PR description's "Never fails the pipeline" guarantee.

  The fix is a simple file-existence guard:

  if command -v node >/dev/null 2>&1 && [ -f /tmp/ado-aw-scripts/ado-script/conclusion.js ]; then
    node /tmp/ado-aw-scripts/ado-script/conclusion.js
  else
    echo "##vso[task.logissue type=warning]conclusion.js unavailable; skipping conclusion reporting"
  fi

  Alternatively, continue_on_error = true on the conclusion bash step achieves the same result with less precision.

⚠️ Suggestions

• scripts/ado-script/src/shared/wit.ts findWorkItemByTitle — The WIQL title-escaping comment from the deleted Rust code (// This title value comes from operator-controlled front-matter configuration...) was important for security audit purposes — it explains why single-quote doubling is safe here and that this is not agent-supplied content. The TypeScript equivalent has no such comment. Worth porting for reviewers and future maintainers.

• title-prefix field name vs. renderTitle behaviour — The field is named title-prefix but renderTitle() uses it as a full title template supporting {pipeline_name}, {pipeline}, and {signal} interpolations. A user supplying "[ado-aw] Failure" gets that as the entire title (not a prefix prepended to the pipeline name). The name title-template or at least a doc clarification would prevent the "prefix" misinterpretation. Minor UX confusion, but worth noting before the front-matter API is widely used.

✅ What Looks Good

• Clean separation of concerns: Stage-3 noop/missing-tool executors are now pure pass-throughs (no ADO HTTP calls), and work-item filing is fully delegated to the Conclusion job. The behavioral change is well-tested in execute.rs (old warning → new plain success).
• DownloadPipelineArtifact@2 correctly has continue_on_error = true — even if the SafeOutputs artifact is missing, the Conclusion job proceeds and logs a warning rather than failing.
• process.exit(0) on crash in conclusion/index.ts ensures the TypeScript bundle itself never fails the pipeline. The only gap is the pre-TypeScript Node.js invocation described above.
• WIQL single-quote escaping (title.replaceAll("'", "''")) is correct.
• The $[dependencies.X.result]-in-EnvValue::Literal-hoisted-to-job-variables pattern is the established ADO approach, and the in-code comments explain the ADO constraint clearly.
• Codemod 0003_flatten_work_item_config with conflict detection and idempotency test is well-structured.
• EnvValue::AdoMacro double-expansion guard (lower.rs) is a nice defensive addition bundled with this PR.

Generated by Rust PR Reviewer for issue #1076 · 317.5 AIC · ⌖ 20.3 AIC · ⊞ 34.1K · ◷

[github-actions]

🔍 Rust PR Review

Summary: Solid architecture, but there's a blocking bug: the documented report-failure-as-work-item key is rejected at compile time by the existing validation gate.

Findings

🐛 Bugs / Logic Issues

• [src/compile/common.rs — validate_safe_outputs_keys] report-failure-as-work-item: false triggers a compile-time "unrecognised tool name" error.
  validate_safe_outputs_keys iterates every key in safe_outputs and rejects anything not in ALL_KNOWN_SAFE_OUTPUTS, NON_MCP_SAFE_OUTPUT_KEYS, DEBUG_ONLY_TOOLS, or the "memory" migration escape-hatch. report-failure-as-work-item is none of these, so any pipeline using:

  safe-outputs:
    report-failure-as-work-item: false

  will fail compilation with "safe-outputs contains unrecognised tool name(s)". The types.rs unit test only exercises parsing, not the validation pass, so this is untested. Fix: add a special case (or add it to NON_MCP_SAFE_OUTPUT_KEYS) in validate_safe_outputs_keys.

• [src/compile/agentic_pipeline.rs — build_conclusion_job] Conclusion job bypasses supply-chain: mirrors.
  The bash download step always fetches from https://github.com/githubnext/ado-aw/releases/... regardless of whether supply-chain: is configured. Pipelines that mirror ado-script.zip via an internal feed for air-gapped environments will have the Conclusion job fail at the download step. The existing ado_script extension's download logic (which is supply-chain:-aware) isn't reused here.

🔒 Security Concerns

• [scripts/ado-script/src/shared/wit.ts — findWorkItemByTitle] WIQL escaping is correct, but the comment in the old Rust code that explained why it's safe (operator-controlled, not agent-controlled) was not ported.
  The ' → '' doubling is the right WIQL defence. Worth adding an inline comment similar to the deleted Rust one clarifying the trust boundary — especially since buildTitle now appends config.pipelineName, which is author-controlled front matter, while the dedup title becomes agent-observable. Not a current vulnerability but a future footgun.

⚠️ Suggestions

• [scripts/ado-script/src/conclusion/index.ts — buildTitle] Title length not validated before ADO API call.
  buildTitle concatenates titlePrefix + " " + pipelineName without bounds checking. ADO work item titles have a 255-character limit; exceeding it causes createWorkItem to throw, which fileSignal catches and logs as a warning. The work item is silently not filed. Consider truncating or rejecting at compile time (e.g. validate_safe_outputs_keys could check string length of title-prefix).

• [src/compile/agentic_pipeline.rs — build_conclusion_job] UseNode@1 and the download bash step lack explicit condition: always().
  Steps within a job with condition: always() still follow the default "run if previous step succeeded" step-level condition. If UseNode@1 fails, the download step is skipped, but conclusion_step has condition: always() and will still run — correctly falling back to the command -v node guard. This is arguably fine, but it is worth noting the asymmetry; adding condition: always() to the download bash step would make the Conclusion job fully robust to node install failures.

✅ What Looks Good

• Correct EXECUTED_NDJSON_FILENAME alignment: ndjson.rs defines "safe-outputs-executed.ndjson" and conclusion/index.ts uses the same constant — no naming divergence.
• Clean noop/missing-tool refactor: removing ADO work-item filing from Stage 3 and delegating it to the always-running Conclusion job is architecturally correct and the behavior change is well-tested in execute.rs.
• WIQL single-quote escaping in wit.ts::findWorkItemByTitle matches the deleted Rust implementation exactly.
• Codemod conflict detection in 0003_flatten_work_item_config.rs correctly bail!s rather than silently overwriting when flat fields and a work-item: block coexist.
• Module rename (safeoutputs → safe_outputs) is systematic; all internal references updated.
• Job result variable hoisting: the $[dependencies.X.result] → $(AW_X_RESULT) pattern is the correct ADO approach for surfacing runtime expressions into step env blocks — well-executed here.

Generated by Rust PR Reviewer for issue #1076 · 410.7 AIC · ⌖ 12.9 AIC · ⊞ 34.1K · ◷

[github-actions]

🔍 Rust PR Review

Summary: Solid implementation of the Conclusion job feature — architecture is well-thought-out and follows existing conventions. Three items worth addressing before merge.

---

Findings

🐛 Bugs / Logic Issues

scripts/ado-script/src/shared/wit.ts:155 — WorkItemReportConfig.includeStats field is dead code

includeStats: boolean is declared in the TypeScript WorkItemReportConfig interface and set to true in buildWorkItemConfig, but fileOrAppendWorkItem never reads it. The stats block is always appended unconditionally in the report builder functions (appendStats). The field mirrors the now-deleted Rust WorkItemReportConfig but has no effect in the TS implementation.

src/compile/agentic_pipeline.rs:1228 — Implicit all-tools opt-in may surprise pipeline authors

When safe-outputs: noop: {} is the only configured tool (triggering the Conclusion job), the TypeScript defaults reportAsWorkItem: true for all diagnostic signal kinds (missing-tool, missing-data, pipeline_failure) — not just noop. No AW_MISSING_TOOL_REPORT_AS_WORK_ITEM env var is emitted for unconfigured tools, so the TS default silently opts them in. Pipelines that configure only noop: will also get work items for missing-tool and missing-data by default. This may or may not be intentional behaviour — it mirrors the gh-aw pattern — but it should be clearly documented in docs/conclusion.md.

⚠️ Suggestions

tests/compiler_tests.rs — No test for Conclusion dependsOn when Teardown exists

test_conclusion_job_is_emitted_with_expected_condition_and_dependencies verifies the no-Teardown path (deps == [Agent, Detection, SafeOutputs]). There's no test for the case where a teardown: block exists — i.e. that Teardown appears in the Conclusion dependsOn list. The has_teardown branch in wire_explicit_dependencies is untested.

scripts/ado-script/src/conclusion/index.ts:193 — extractNamedValue regex fallback is loose

The fallback pattern /tool[_ -]?name\s*:\s*([^\r\n,;]+)/i applied to sanitized-but-agent-observable context strings could match unintended substrings (e.g. "no tool name: available in registry" → extracts "available in registry"). The field tool_name is already in the manifest schema and carries the right value when populated; the regex path adds noise. Consider tightening to an exact match or removing it.

✅ What Looks Good

• WIQL injection: single-quote doubling in findWorkItemByTitle is the correct WIQL escape; the comment accurately describes why the input is trusted (compile-time front matter) while keeping the escape as defence-in-depth.
• buildTitle title truncation to 255 chars: correctly prevents ADO API rejections for long titlePrefix + pipelineName combinations while preserving dedup stability (same inputs → same title).
• ADO runtime-expression hoisting pattern: $[dependencies.X.result] goes into variables: via EnvValue::Literal; step env references via EnvValue::PipelineVar ($(AW_AGENT_RESULT)). Matches the project's established pattern and avoids the $[...]-in-step-env gotcha caught by the existing test.
• Graceful degradation in conclusion script: the command -v node guard and continue_on_error: true on the artifact download ensure the Conclusion job never fails the pipeline even if Node or the artifact is unavailable.
• Codemod test coverage: all six tests for 0003_flatten_work_item_config are thorough, including conflict detection and idempotency.
• fileSignal missing-project guard: early return with a warning when SYSTEM_TEAMPROJECT is unset is the correct behaviour rather than letting the SDK throw.

Generated by Rust PR Reviewer for issue #1076 · 347.7 AIC · ⌖ 12.4 AIC · ⊞ 35.7K · ◷