fix(safe-outputs): make create-work-item body field configurable

[Copilot]

Summary

create-work-item always wrote agent-generated body content to System.Description, which makes Bug work items appear blank in processes whose Bug form renders Microsoft.VSTS.TCM.ReproSteps instead. It also escaped body HTML before storing the field as Markdown, producing unreadable content.

• Work item body field
  • Added safe-outputs.create-work-item.description-field.
  • Defaults Bug to Microsoft.VSTS.TCM.ReproSteps.
  • Defaults all other work item types to System.Description.
  • Validates configured field refs before JSON Patch construction.

safe-outputs:
  create-work-item:
    work-item-type: Bug
    description-field: Microsoft.VSTS.TCM.ReproSteps

• Body sanitization

  • Preserves ordinary inline HTML in Markdown descriptions.
  • Still strips active HTML constructs, event handlers, dangerous URL protocols, and unclosed unsafe tags.

• Docs and coverage

  • Documented description-field and Markdown body semantics.
  • Added focused tests for default selection, custom field validation, MCP proposal output, executor patch payload, and sanitizer behavior.

Test plan

• cargo test create_work_item
• cargo test sanitize_markdown
• cargo test validate_ado_work_item_field_ref

• Fixes [agent-issue]: create-work-item hardcodes System.Description, so Bug work items are filed blank #1924

[azure-pipelines]

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

[jamesadevine]

/review

[github-actions]

✅ PR Security Reviewer completed the security review.

🔒 Security review by PR Security Reviewer

[github-actions]

Reviewed for ado-aw's compiler-contract invariants (front-matter grammar, safe-output schema/newtype rules, generated-artifact drift, docs sync).

• Front-matter grammar: description_field: Option<AdoWorkItemFieldRef> with #[serde(default)] is backward compatible — no codemod needed.
• Newtype rule: correctly followed — description-field uses the new AdoWorkItemFieldRef validated newtype (src/secure.rs) backed by validate_ado_work_item_field_ref (src/validate.rs), not a raw String. This also protects the /multilineFieldsFormat/{description_field} JSON Patch path from Pointer-syntax corruption.
• Docs sync: docs/front-matter.md and docs/safe-outputs.md were both updated with the new description-field option and defaulting behavior — good.
• No generated-artifact drift: this PR only touches src/ and docs/; no ado-script bundles, IR/gate schema, or .lock.yml files are implicated, so no drift check applies here.

No merge-blocking compiler-contract issues found. This is a well-scoped, correctly-typed change.

🏗️ Compiler contract review by Compiler Contract Reviewer · auto · 41.4 AIC · ⌖ 2.3 AIC · ⊞ 12.8K
_{Comment /review to run again}

[github-actions]

Solid test coverage overall for the new description-field config and the HTML-preserving sanitize_markdown path (defaults, Bug override, invalid value rejection, executor E2E, sanitizer preserve/neutralize). Two gaps worth addressing before merge:

1. No test covers description-field colliding with custom-fields or System.Title, which produces a duplicate JSON Patch path that ADO will likely reject at runtime — this failure mode is currently undetectable except in production.
2. test_sanitize_markdown_neutralizes_active_html bundles ~15 distinct attack vectors into one test; a regression in any single vector is masked by the rest passing, making CI failures hard to localize.

Neither is merge-blocking on its own, but the first is a real correctness gap worth at least a regression test.

🧪 Test quality analysis by Test Quality Sentinel · auto · 60.8 AIC · ⌖ 2.84 AIC · ⊞ 11.3K
_{Comment /review to run again}

[github-actions]

Rust code quality review

No merge-blocking Rust defects found in src/mcp.rs, src/safe_outputs/create_work_item.rs, src/sanitize.rs, src/secure.rs, src/validate.rs.

💡 Notes

• description_field_for correctly derives its fallback to the validated AdoWorkItemFieldRef type, and validate_ado_work_item_field_ref rejects .///~ characters that would otherwise corrupt the JSON Patch pointer path built in field_op — good defensive design.
• Minor, non-blocking: strip_dangerous_html_tags and strip_event_handler_attrs_in_tags in src/sanitize.rs each re-implement an almost identical tag-scanning loop around html_tag_end; a shared helper that takes a per-tag transform closure would remove the duplication, but it's not a correctness issue and isn't worth a blocking comment.
• The rust-critic sub-agent returned no output for this diff after repeated polling; I completed the review from my own pass only.
• Error handling, casts, path handling, and concurrency all looked sound in the changed lines — anyhow::Result/.context() used consistently, no new unwrap()/expect() on agent-input paths, no lossy casts introduced.

🦀 Rust code quality review by Rust Code Quality Reviewer · auto · 79.3 AIC · ⌖ 2.23 AIC · ⊞ 11.4K
_{Comment /review to run again}

[github-actions]

One merge-blocking finding: the new strip_dangerous_html_tags HTML sanitizer in src/sanitize.rs is single-pass and vulnerable to the classic nested-tag bypass (e.g. <scr<script>ipt>alert(1)</scr</script>ipt> reconstructs a live <script> tag after stripping). I verified this against a standalone extraction of the exact diff algorithm. Since sanitize_markdown is now used for work-item descriptions, and description-field routes Bug items to a Markdown-rendered field, this is a stored-XSS-class regression introduced by this PR (the old sanitize path fully HTML-escaped, so this preserving-HTML replacement needs to close this gap before merge). See inline comment for details and a suggested fix (loop the tag-stripping pass to a fixed point, or use a real tokenizer).

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

[jamesadevine]

@copilot implement suggestions and resolve blocking findings.

[Copilot]

@copilot retain code fence preservation

Fixed in b14f429. sanitize_markdown now preserves Markdown protected ranges while neutralizing dangerous HTML outside them, with a regression test for fenced code blocks. cargo test sanitize_markdown, code review, and CodeQL all passed.

[jamesadevine]

/review

[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 review — no blocking findings

Reviewed the create-work-item description-field change against the ado-aw compiler contracts:

• Identifier newtype: description-field and custom-fields keys are typed as AdoWorkItemFieldRef (validated_string! macro in src/secure.rs), not raw String — satisfies the secure.rs contract for identifier-like fields used in JSON Patch paths.
• Validation coverage: validate_ado_work_item_field_ref runs at Deserialize time (via the macro-generated Deserialize impl), so a malformed field ref cannot reach field_op/JSON Patch construction.
• Runtime duplicate-field guard: validate_patch_fields catches case-insensitive collisions between description-field, area-path, tags, and custom-fields before the patch is built — good defense given fields are all configurable strings.
• Docs: docs/front-matter.md and docs/safe-outputs.md were both updated with the new description-field option and its per-work-item-type defaulting behavior.
• Sanitization: sanitize_markdown is a deliberate, narrower alternative to sanitize_text for Markdown bodies (preserves inline HTML, still strips active HTML/handlers/dangerous URL protocols) — used consistently in both mcp.rs (MCP proposal write) and create_work_item.rs (SanitizeContent executor path), so the same sanitization applies whether validated at proposal-write time or Stage 3 output-apply time.
• Codegen/drift: no changes to filter_ir.rs, the Fact enum, IR, or ado-script bundles in this diff, so no codegen or bundle drift applies here.
• Tests: field-ref validation, config defaulting, MCP sanitized-proposal output, and executor patch-payload construction are all covered.

No half-missing contract found. Nothing to request changes on from the compiler-contract perspective.

🏗️ Compiler contract review by Compiler Contract Reviewer · auto · 44.9 AIC · ⌖ 2.71 AIC · ⊞ 12.8K
_{Comment /review to run again}

[github-actions]

Solid test coverage overall: default/custom description-field selection, collision validation across all field kinds, sanitizer preservation vs. stripping of HTML, and an executor-level HTTP round-trip via wiremock. Two gaps worth closing before merge, both advisory (not blocking):

1. The description-field collision tests only exercise the Bug → ReproSteps default path; the System.Description default path (the common case) has no equivalent custom-fields collision test.
2. The dangerous-HTML fixed-point stripping loop is only proven against <script> nesting; the other 9 tag names it guards (iframe, svg, style, etc.) aren't tested against the same folding attack.

No weakened/removed assertions detected, and the new validate_ado_work_item_field_ref and JSON-patch collision paths look well covered on the error side.

🧪 Test quality analysis by Test Quality Sentinel · auto · 54.4 AIC · ⌖ 1.84 AIC · ⊞ 11.3K
_{Comment /review to run again}

[github-actions]

This diff introduces sanitize_markdown to preserve inline HTML in work-item descriptions instead of fully HTML-escaping them, and adds a matching event-handler/dangerous-tag stripper. I found a stored-XSS bypass in that stripper (inline comment on src/sanitize.rs:264): the event-handler regexes require a literal space or < before onXXX=, so <img/onerror=...> (slash separator) and similarly-crafted on* attributes on non-blocklisted tags (img, a, body, etc.) pass through untouched. Everything else in the diff (field-ref validation, token/config handling, dangerous-tag nested-stripping loop) looked sound.

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

[github-actions]

Rust-quality pass on the changed files (src/mcp.rs, src/safe_outputs/create_work_item.rs, src/sanitize.rs, src/secure.rs, src/validate.rs). Solid error handling throughout (validated newtypes, anyhow::ensure!, no risky unwrap()/expect() on untrusted paths — the one expect() in the tag-stripping loops is provably safe since it follows an index < input.len() check). Left three advisory comments: a possible quadratic-time worst case in the new fixed-point HTML tag stripper running before the content-size cap, a comment left orphaned by code relocation, and a HashMap-iteration-order inconsistency between custom-field validation (sorted) and patch construction (unsorted). None of these are correctness-breaking for the primary use case, so COMMENT rather than blocking.

The rust-critic sub-agent did not return output in time; findings above are from my own pass only.

Themes reviewed

• Error handling / anyhow context usage — clean.
• New AdoWorkItemFieldRef validated type and validate_ado_work_item_field_ref — well-tested, correctly gates dot-segment identifiers.
• HashMap<AdoWorkItemFieldRef, String> key-type migration — behaviorally sound, but ordering divergence noted above.
• New sanitize_markdown HTML-stripping pipeline — algorithmically more expensive than the existing escape_html_tags; flagged the ordering-vs-size-limit concern.

🦀 Rust code quality review by Rust Code Quality Reviewer · auto · 129.5 AIC · ⌖ 1.91 AIC · ⊞ 11.4K
_{Comment /review to run again}

[jamesadevine]

@copilot address final review comments

[Copilot]

@copilot address final review comments

Addressed in 48b6a6f through a287f33. The follow-up strips slash-separated event-handler attributes, pre-caps Markdown before the expensive HTML pass, makes custom-field patch ordering deterministic, and adds the requested collision/folded-tag sanitizer coverage. cargo test sanitize_markdown, cargo test create_work_item, secret scanning, and CodeQL passed.

[jamesadevine]

/review

[github-actions]

✅ PR Security Reviewer completed the security review.

Caution

agentic threat detected
Threat detection flagged this output in warn mode. Manual review is REQUIRED before any follow-up automation.

Details

Potential security threats were detected in the agent output.

Review the workflow run logs for details.

🔒 Security review by PR Security Reviewer

[github-actions]

Solid test coverage overall: the new description-field config, patch-field collision validation, and the HTML-preserving markdown sanitizer all have targeted unit tests plus one mocked-HTTP executor test. Found a few gaps that would let regressions ship silently — none are blocking, all are advisory additions to widen coverage on the case-insensitive Bug-type match and the full collision matrix.

🧪 Test quality analysis by Test Quality Sentinel · auto · 65.7 AIC · ⌖ 2.65 AIC · ⊞ 11.3K
_{Comment /review to run again}

[github-actions]

Rust review — no blocking issues found

Reviewed src/mcp.rs, src/safe_outputs/create_work_item.rs, src/sanitize.rs, src/secure.rs, src/validate.rs.

• AdoWorkItemFieldRef validation is strict and correctly guards against JSON Pointer path corruption in /fields/<ref>.
• sorted_custom_fields fixes what would otherwise be nondeterministic HashMap iteration in generated JSON Patch output — good catch, and it's covered by test_sorted_custom_fields_orders_case_insensitively.
• validate_patch_fields correctly detects field collisions (title/description/custom-fields) before use, avoiding silent data loss on the ADO patch document.
• The new sanitize_markdown/dangerous-tag stripping logic in sanitize.rs (fixed-point loop over strip_dangerous_html_tags, event-handler-attribute stripping, protected fenced-code ranges) is thorough and has strong adversarial test coverage (nested/folded tag reconstruction, attribute-span folding, unclosed tags).
• No unwrap()/expect() on user-reachable paths; error handling uses anyhow::ensure!/bail! with actionable messages throughout.
• cargo build succeeds cleanly.

The Compiler Contract reviewer owns the front-matter/schema/docs side of this change; nothing further from a pure Rust-engineering standpoint.

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.

🦀 Rust code quality review by Rust Code Quality Reviewer · auto · 66.5 AIC · ⌖ 5.98 AIC · ⊞ 11.4K
_{Comment /review to run again}

[github-actions]

Compiler-contract review: front-matter grammar, safe-output docs, and the new AdoWorkItemFieldRef newtype all look correctly wired (properly uses validated_string!/src/validate.rs, not a raw String). Tests are thorough and pass locally (cargo test --bin ado-aw create_work_item). One finding posted inline: the new validate_patch_fields collision check only runs in the Stage 3 executor, not in the compile-time validate_work_item_assignment_outputs_config, so a misconfigured description-field/custom-fields collision is discovered only after a pipeline runs, not at ado-aw compile/lint time.

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.

🏗️ Compiler contract review by Compiler Contract Reviewer · auto · 107.6 AIC · ⌖ 2.45 AIC · ⊞ 12.8K
_{Comment /review to run again}

[github-actions]

Caution

agentic threat detected
Threat detection flagged this output in warn mode. Manual review is REQUIRED before any follow-up automation.

Details

Potential security threats were detected in the agent output.

Review the workflow run logs for details.

Reviewed the sanitize/validate/create-work-item changes. The new AdoWorkItemFieldRef typed newtype, validate_ado_work_item_field_ref, and the config sanitization (SanitizeConfig impl covering all string fields including custom-field keys/values and artifact-link) look correctly scoped and don't weaken any existing control.

One finding worth blocking on: sanitize_markdown (new src/sanitize.rs function used by create-work-item descriptions) preserves raw HTML while still relying on the pre-existing sanitize_url_protocols, which does a literal substring search for (redacted) That check is bypassable with an embedded whitespace/control character in the scheme (java\tscript:), and because the surrounding HTML is no longer escaped to inert entities (unlike the original sanitize()path), this bypass is now live rather than latent — a malicious agent-authored work item description could smuggle a(redacted) URI into a rendered <a href>/<img src>. See inline comment for a reproduction and suggested fix (normalize/strip whitespace from the scheme before matching, or operate on parsed attribute values).

Everything else — token handling, custom-field key typing, patch-field duplicate-field validation, and the dangerous-tag/event-handler stripping logic — is sound.

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

Superseded by updated review from same workflow.