fix(safeoutputs): stop dropping GitHub issue tool configs in Stage 3 - #1804
Merged
Conversation
`ExecutionContext::get_tool_config` stripped the compiler-injected `require-approval` key but not `staged`, which Stage 3 also injects into every tool config (`main.rs` for the `--source` path, `compile/custom_tools.rs` for the `--resolved-config` path production actually uses). `CreateGithubIssueConfig` and `SetGithubIssueTypeConfig` are the only safe-output configs declared `deny_unknown_fields`, so deserialization failed — and the error was swallowed by `.ok().unwrap_or_default()`, silently replacing the operator config with `Default::default()`. Observable effects: * `target-repo` ignored, so Stage 3 failed outright on non-GitHub-backed ADO builds with "target-repo is required when the Azure DevOps pipeline source is not GitHub" * `title-prefix` never applied; static `labels`/`assignees` dropped * `allowed-labels` emptied, so default-deny rejected *every* agent label * `require-temporary-id` never enforced; `max` budget override ignored * `set-github-issue-type.allowed` never gated anything — an empty list is default-allow, so this one failed OPEN Strip `staged` alongside `require-approval`, and log a warning instead of silently defaulting so a future config-shape mismatch is visible rather than presenting as a mysterious runtime failure. The existing wiremock tests could not catch this: they build an `ExecutionContext` directly with a `tool_configs` map that has no `staged` key, i.e. a shape that never occurs in production. The added regression tests assert an operator config survives both injected keys. Found while adding deterministic executor-e2e coverage for these tools. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: cefea537-5177-4dfd-8849-bcc2caff2845
|
Azure Pipelines: Successfully started running 1 pipeline(s). 1 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
ExecutionContext::get_tool_configstripped the compiler-injectedrequire-approvalkey but notstaged, which Stage 3 also injects into every tool config:src/main.rs— the--sourcepathsrc/compile/custom_tools.rs— the compiler-generated--resolved-config, which is what production usesCreateGithubIssueConfigandSetGithubIssueTypeConfigare the only safe-output configs declared#[serde(deny_unknown_fields)], so deserialization failed. The error was then swallowed:The operator config was silently replaced with
Default::default().Impact
target-repotitle-prefixlabels,assigneesallowed-labelsrequire-temporary-idmaxset-github-issue-type.allowedThe last row is the security-relevant one: the label allowlist failed closed, but the issue-type allowlist failed open.
Reproduction
Using the exact config shape the compiler emits:
Before:
create-github-issue - ✗ - target-repo is required when the Azure DevOps pipeline source is not GitHubAfter:
create-github-issue - ✗ - Failed to file GitHub issue (HTTP 401 Unauthorized)— i.e. the config is honoured and the call reaches GitHub (401 is just the bogus token).Removing only
"staged": falsefrom the config also fixed it before this change, which isolates the cause.The fix
stagedalongsiderequire-approval.warn!instead of silently defaulting, so a future config-shape mismatch is visible rather than presenting as a mysterious runtime failure.Why no existing test caught it
The wiremock tests construct an
ExecutionContextdirectly with atool_configsmap that has nostagedkey — a shape that never occurs in production.The two added regression tests assert an operator config survives both injected keys, and are mutation-verified: reverting the one-line
object.remove("staged")turns both red.Testing
cargo test --bin ado-aw— 2828 passedcargo clippy --bin ado-aw— cleancargo fmtdrift (4 pre-existing diffs in this file are unchanged)Found while adding deterministic executor-e2e coverage for these tools, which follows in a stacked PR.