feat: add update-work-item safe output#65
Conversation
|
@copilot We should come up with a way e.g. tag-prefix which allows editing of work items with only tags in the list. |
Done in 8045891. Added safe-outputs:
update-work-item:
title: true
tag-prefix: "agent-" # only update work items that have a tag starting with "agent-"When both Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
Co-authored-by: jamesadevine <4742697+jamesadevine@users.noreply.github.com>
Co-authored-by: jamesadevine <4742697+jamesadevine@users.noreply.github.com>
jamesadevine
force-pushed
the
copilot/update-work-item-safeoutput
branch
from
8045891 to
2256543
Compare
|
/rust-review |
🔍 Rust PR ReviewSummary: Looks good overall — well-structured, solid field-level permission checks, and good test coverage — but there are two correctness issues worth addressing before merge. Findings🐛 Bugs / Logic Issues
|
|
✅ Rust PR Reviewer completed successfully! |
🔍 Rust PR ReviewSummary: Mostly well-implemented — good test coverage, proper sanitization, and field-level permission guards — but there is one correctness bug and a couple of minor concerns worth addressing. Findings🐛 Bugs / Logic Issues
|
|
@copilot src/tools/update_work_item.rs:426-430 — /multilineFieldsFormat op breaks older ADO Server instances
Missing integration test fixture — the PR adds unit tests but there's no fixture in tests/fixtures/ exercising a round-trip compilation of a markdown file that includes update-work-item in safe-outputs. At minimum a case verifying that permissions.write is required (exercising the validate_write_permissions path) would be valuable. |
Co-authored-by: jamesadevine <4742697+jamesadevine@users.noreply.github.com> Agent-Logs-Url: https://github.com/githubnext/ado-aw/sessions/6464dd4d-6192-48d9-b5fc-ac5bdbe709b7
All four issues addressed in 197a27f:
Security summary: no new vulnerabilities introduced; changes are purely removals and opt-in additions. Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
|
/rust-review |
|
✅ Rust PR Reviewer completed successfully! |
🔍 Rust PR ReviewSummary: Needs changes — solid implementation overall, but two substantive issues worth addressing before merge. Findings🐛 Bugs / Logic Issues
🔒 Security Concerns
|
|
🐛 Bugs / Logic Issues When update_wi_count > update_config.max, the code does return Err(...) from execute_safe_outputs. This causes the entire safe-output processing job to fail — PRs, wiki pages, and every other queued output are discarded along with the offending work-item updates. Compare with create_work_item, which uses write_safe_output_file_with_maximum at Stage 1 (MCP write time) to silently cap at the limit — excess outputs simply aren't written, so Stage 2 never sees them. Either mirror that pattern for update-work-item (preferred, keeps Stage 2 clean), or change the Stage 2 check to process only the first max entries rather than aborting everything. 🔒 Security Concerns patch_doc.push(replace_field_op("System.Tags", tags.join("; "))); let safe_tags: Vec<&str> = tags.iter()
Fields are sanitized in mcp.rs before try_into(), then sanitized again via Sanitize::sanitize_fields() inside execute_sanitized. Idempotent, so not a bug, but one layer is redundant. Since other tools follow the same pattern (sanitize at MCP layer), this is consistent — but worth noting the Stage 2 sanitization is the real safety net and the MCP-layer one can be dropped for simplicity. UpdateWorkItemConfig.status vs UpdateWorkItemParams.state naming asymmetry The config key is named status (YAML: status: true) but the agent parameter is named state. The struct-level comment explains the discrepancy, but the error message "set 'status: true' in safe-outputs" when a state update is blocked is the only hint in the runtime path. Consider renaming the config field to state to match the agent API, or at least make the mismatch more prominent in the schema docs |
Co-authored-by: jamesadevine <4742697+jamesadevine@users.noreply.github.com> Agent-Logs-Url: https://github.com/githubnext/ado-aw/sessions/0199ff90-689d-4eb9-b34b-f38b36709424
|
... Both bugs fixed in 3824b23:
Tag injection (update_work_item.rs): Added a check in The double-sanitization observation and the Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
Adds an
update-work-itemsafe output tool for modifying existing ADO work items, with fine-grained front matter controls mirroring theupdate-issuepattern from GitHub Agentic Workflows.Configuration
Each updatable field must be explicitly opted-in. All default to
false/disabled:Key design decisions
targetaccepts"*"(any ID) or an integer (lock to one item) — rejects at execution time if the agent-supplied ID doesn't match; unrecognised pattern values emit awarn!log rather than silently blocking all updatestitle-prefixissues a GET before the PATCH to verify the current title; rejected if it doesn't matchtag-prefixissues a GET before the PATCH to verify the current tags; rejected if no tag starts with the prefix. ADO tags are semicolon-separated and each is trimmed before comparisontitle-prefixandtag-prefixare configured, a single GET is issued and both checks are applied before the PATCHmarkdown-bodyisfalseby default for broad ADO Server compatibility. Set totrueto emit the/multilineFieldsFormat/System.Description: "Markdown"patch op (requires ADO Services or Server 2022+)work_item_typeis not supported —System.WorkItemTypeis read-only via the WIT PATCH API and the endpoint returns HTTP 400 if it is includedmaxis enforced at Stage 2 by skipping excessupdate-work-itementries (adding afailureresult per skipped entry) rather than aborting the entire batch — all other queued outputs (PRs, wiki pages, etc.) continue to executefailureresult (not an error) with a clear message pointing to the missing config keyWRITE_REQUIRING_SAFE_OUTPUTS— compilation fails if nopermissions.writeservice connection is configuredAgent parameters
idu64titleOption<String>bodyOption<String>markdown-body: truein config for Markdown renderingstateOption<String>"Active","Resolved"area_pathOption<String>iteration_pathOption<String>assigneeOption<String>tagsOption<Vec<String>>At least one field must be non-null; validation enforced at Stage 1 (MCP) and sanitization runs at both Stage 1 and Stage 2.
📱 Kick off Copilot coding agent tasks wherever you are with GitHub Mobile, available on iOS and Android.