Examining safe output for GitHub PR updates - #2221
jamesadevine with Copilot wants to merge 4 commits into
Conversation
Co-authored-by: jamesadevine <4742697+jamesadevine@users.noreply.github.com>
Co-authored-by: jamesadevine <4742697+jamesadevine@users.noreply.github.com>
Co-authored-by: jamesadevine <4742697+jamesadevine@users.noreply.github.com>
Co-authored-by: jamesadevine <4742697+jamesadevine@users.noreply.github.com>
|
Azure Pipelines: Successfully started running 2 pipeline(s). There may be pipelines that require an authorized user to comment /azp run to run. |
|
✅ Rust Code Quality Reviewer completed the Rust code quality review.
|
|
✅ Compiler Contract Reviewer completed the compiler contract review.
|
|
✅ Test Quality Sentinel completed the test quality analysis.
|
|
✅ PR Security Reviewer completed the security review.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Compiler contract review — update-pull-request
Contract check: this drift is real, everything else lines up.
AGENTS.md'ssrc/safe_outputs/file tree (around theupdate_pr.rs/
update_wiki_page.rslisting) does not include the new
update_pull_request.rsmodule. That tree is the map every future agent
reads to orient itself in this crate — add a line for it, e.g.between│ │ ├── update_pull_request.rsupdate_pr.rsandupdate_wiki_page.rs.- Front-matter grammar, safe-output registration (
ALL_KNOWN_SAFE_OUTPUTS,
mod.rsmodule list,execute.rsdispatch,mcp.rstool registration,
inspect/catalog.rsdescription), config validation wiring in
compile/common.rs, anddocs/safe-outputs.mdare all present and
consistent with the siblingupdate-pr/update-github-issuetools. repository: Option<String>mirrors the existing convention in
update_pr.rs/update_github_issue.rs(validated via
reject_pipeline_injection, not asrc/secure.rsnewtype), so it is not a
new gap introduced by this PR.replace-islandbehavior intentionally diverges from
update_github_issue.rs's strict "exactly one matching pair or fail"
semantics — this one falls back to appending a fresh island when markers are
missing/duplicated instead of failing. That's a reasonable UX choice for a
brand-new PR description, but worth a one-line doc callout so it isn't read
as an inconsistency with the GitHub-issue tool's stricter contract.
No breaking front-matter change (all new fields are Option/defaulted), no
codegen/lock drift, and cargo build succeeds with the new module compiled
in. Non-blocking — the only actionable fix is the AGENTS.md tree entry.
🏗️ Compiler contract review by Compiler Contract Reviewer · auto · 119.2 AIC · ⌖ 2.17 AIC · ⊞ 11.3K
Comment /review to run again
There was a problem hiding this comment.
Test Quality Review — update-pull-request safe output
The new src/safe_outputs/update_pull_request.rs (891 lines) has 4 tests, all following happy paths (or a single simple failure guard). Several failure branches that gate real security/config behavior in execute_impl are completely untested:
required-labels/required-title-prefixrejection (validate_filters) — the only test using these config keys satisfies them; the rejection path that actually blocks the agent isn't exercised.- Request-level
update_branch: truerejection — distinct from (and easily confused with) the config-levelupdate-branch: truecheck, which is tested. - HTTP failure branches on both the GET (
fetch_pr) and PATCH (update) calls — no test asserts that a non-2xx response from Azure DevOps produces a failure result rather than silently succeeding. allowed-repositoriesdenial — the allow-list gate that restricts which repo alias an agent may target has no negative test.resolve_id's fixed-target: <id>branch, including the requested-vs-configured mismatch case, is untested (only"*"and implicitly"triggering"are covered).
None of these are exotic edge cases — they're the primary failure/security-gate paths of a new write-capable safe output, and each would ship silently broken if regressed. Filed as 6 inline comments with suggested test shapes (mock-server based, following the existing wiremock pattern already used in this file).
No existing assertions were weakened or removed, so this is advisory (COMMENT) rather than blocking — but I'd strongly recommend closing at least the required-labels/allowed-repositories gaps before merge, since those are the actual authorization boundaries this tool relies on.
🧪 Test quality analysis by Test Quality Sentinel · auto · 77 AIC · ⌖ 6.2 AIC · ⊞ 9.8K
Comment /review to run again
| .any(|label| label.name.eq_ignore_ascii_case(required)) | ||
| }) | ||
| .collect(); | ||
| if !missing.is_empty() { |
There was a problem hiding this comment.
No test exercises validate_filters's rejection path — required-labels/required-title-prefix are a config-level approval gate (a PR must carry a label or title prefix before the agent can touch it), and their failure branch is entirely unverified. The one test that sets these config keys (updates_triggering_pr_title_and_body) only exercises the case where the PR already satisfies them, so a regression that silently drops this gate (e.g. missing.is_empty() inverted, or the prefix check skipped) would ship undetected.
💡 Suggested test
Add a case where the mocked PR is missing a required label or has a non-matching title, and assert execute_sanitized returns success: false with a message naming the missing label/prefix, and that no PATCH request was sent to the mock server.
| "update-pull-request field 'body' is not enabled by configuration", | ||
| )); | ||
| } | ||
| if self.update_branch == Some(true) { |
There was a problem hiding this comment.
The update_branch == Some(true) request-level rejection is untested. config_matches_gh_aw_shape_but_rejects_update_branch_true only covers the config field (update-branch: true in front matter), which is a separate check (validate_update_pull_request_config) from this request-level field on UpdatePullRequestResult/params. These two guards are easy to conflate and a regression in this one (request-level) would not be caught by the existing test.
💡 Suggested test
Call execute_sanitized with update_branch: Some(true) in the params and assert failure with the "not supported for Azure DevOps PRs" message, distinct from the config-level test.
| .send() | ||
| .await | ||
| .context("Failed to fetch Azure DevOps pull request")?; | ||
| if !response.status().is_success() { |
There was a problem hiding this comment.
The non-success HTTP response branch of fetch_pr (e.g. Azure DevOps returning 404/401) is never exercised by a test. All current wiremock-based tests either return 200 or never reach the network call. A regression that swallows the error or misformats the failure message would go unnoticed.
💡 Suggested test
Mock the GET endpoint with ResponseTemplate::new(404) and assert execute_sanitized returns success: false with a message containing the PR id and status code, and that the PATCH mock (if registered) received zero requests.
| .send() | ||
| .await | ||
| .context("Failed to update Azure DevOps pull request")?; | ||
| if !response.status().is_success() { |
There was a problem hiding this comment.
Same gap on the write side: the PATCH failure branch (non-success response from the update call) has no test. Given this is the actual mutating request, a silently-swallowed failure here (e.g. reporting success despite a 4xx/5xx) is a more severe regression than the read-side gap.
💡 Suggested test
Mock the PATCH endpoint with a non-2xx ResponseTemplate and assert the execution result reports failure with the HTTP status embedded.
| Err(result) => return Ok(result), | ||
| }; | ||
| let repo_alias = self.repository.as_deref().unwrap_or("self"); | ||
| if !config.allowed_repositories.is_empty() |
There was a problem hiding this comment.
The allowed-repositories allow-list rejection is untested. This is a security-relevant boundary (restricting which repo alias the agent may target), and nothing verifies that a repository value outside the configured list is actually rejected before any network call is made.
💡 Suggested test
Configure allowed-repositories: ["other"], call with repository: None (defaulting to "self"), and assert failure naming the disallowed alias with zero requests reaching the mock server.
| .requested_id() | ||
| .map_err(|error| ExecutionResult::failure(error.to_string()))?; | ||
| match &config.target { | ||
| UpdatePullRequestTarget::Id(id) => { |
There was a problem hiding this comment.
resolve_id's UpdatePullRequestTarget::Id(id) branch — including the mismatch case where a requested pull_request_id disagrees with the configured fixed target — has no test. Only the "*" and (implicitly, via the happy-path test) "triggering" targets are exercised.
💡 Suggested test
Configure target: 42 and call with pull_request_id: 7, asserting failure with a message naming both the requested and configured IDs — and separately, a case with pull_request_id: 42 or none, asserting success.
There was a problem hiding this comment.
Security review — no regressions found
Reviewed the new update-pull-request safe output (src/safe_outputs/update_pull_request.rs) and its wiring through mcp.rs/execute.rs/common.rs/types.rs.
- Agent-controlled
title/bodygo through the standardSanitizeContentpipeline (sanitize_text), which includes pipeline-command neutralization (##vso[...]), HTML escaping, and mention/bot-trigger neutralization, before ever reaching the ADO PATCH payload — no injection path found. repository,allowed-repositories,required-labels, andrequired-title-prefixare validated withreject_pipeline_injectionat compile time (validate_update_pull_request_config), matching the convention used by sibling safe outputs.- The write-capable token flows only through the existing
ctx.access_token/resolve_repo_namepath, identical toupdate_pr.rs,submit_pr_review.rs, andresolve_pr_thread.rs— this PR does not weaken or bypass any existing check (it does not use the cross-organization-awareresolve_repository_write_target, but neither do those three existing siblings, so this is pre-existing parity, not a new regression). target: "*"allowing the agent to specify an arbitrarypull_request_idis explicit, operator opt-in configuration mirroringupdate-pr/update-github-issuesemantics, not a default-on capability.- The PATCH payload is built from an explicit allow-list (
title,description) only — no arbitrary field pass-through.
No exploitable weakening of the Stage 1/Stage 3 trust boundary or the sandbox network boundary was found in this diff. Not requesting changes.
🔒 Security review by PR Security Reviewer · auto · 128.1 AIC · ⌖ 2.18 AIC · ⊞ 10.2K
Comment /review to run again
There was a problem hiding this comment.
Rust code quality review for the new update-pull-request safe output.
Note: the rust-critic sub-agent failed (400 model "gpt-5.4-mini" is not accessible), so this review reflects only my own pass over the changed lines.
Overall the new file is well-structured (typed IDs, clear separation of params/config/executor, good error messages), and the change compiles cleanly. One real correctness inconsistency found in the allow-list check vs. repository resolution; flagged inline. No merge-blocking defects otherwise, so filing as COMMENT rather than REQUEST_CHANGES.
Themes considered but not flagged
- Duplicated field lists between
UpdatePullRequestParams/UpdatePullRequestResultand the reconstruction inrequested_id()/execute_impl()— a maintenance burden if a field is added later, but not a functional bug today, so left out of the 10-comment budget. - Error-body echoing (
response.text().await) into user-facing failure messages is consistent with existing precedent across the safe-outputs module (e.g.update_pr.rs), so not flagged as new. config.max/footer/sync-stack fields are consumed generically by the framework layer, not this file, so left to the compiler-contract reviewer if relevant.
🦀 Rust code quality review by Rust Code Quality Reviewer · auto · 188.1 AIC · ⌖ 2.04 AIC · ⊞ 10.1K
Comment /review to run again
| .allowed_repositories | ||
| .iter() | ||
| .any(|allowed| allowed == repo_alias) | ||
| { |
There was a problem hiding this comment.
Allowed-repositories check bypasses alias normalization used by the resolution call eight lines below
A misconfigured or differently-cased alias can pass resolve_repo_name (called right after) via canonical_repository_alias's case-insensitive / trailing-segment matching, yet fail this exact-match allowed_repositories.iter().any(|allowed| allowed == repo_alias) check.
💡 Why this matters and a fix
resolve_repo_name (line 638) resolves self.repository through canonical_repository_alias, which matches case-insensitively and also by the trailing repo-name segment of a configured value (src/safe_outputs/mod.rs::canonical_repository_alias). This pre-check instead does a raw == comparison against the literal alias strings in config.allowed_repositories.
Concretely: if an operator configures allowed-repositories: [MyRepo] and the agent supplies repository: myrepo (a value canonical_repository_alias would legitimately resolve), this check rejects it with "not in the allowed-repositories list" even though the equivalent value would otherwise resolve correctly. Since the allow-list is a security control gating which repo gets fetched/patched, having two different matching semantics for the same value undermines its reliability.
Fix: reuse canonical_repository_alias(repo_alias, ctx) for the allow-list membership test instead of comparing raw strings with different semantics than the resolution step:
let resolved_alias = crate::safe_outputs::canonical_repository_alias(repo_alias, ctx);
if !config.allowed_repositories.is_empty()
&& !resolved_alias.as_deref().is_some_and(|alias| config.allowed_repositories.iter().any(|allowed| allowed == alias))
{ ... }
|
Consolidated into #2222, which now contains both Azure DevOps PR safe outputs plus the combined review fixes and tests. |
Pull request created by AI Agent