test(executor-e2e): cover create-github-issue and set-github-issue-type - #1805
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
`create-github-issue` and `set-github-issue-type` shipped with zero runtime coverage: their only proof was a wiremock unit test, and the smoke-suite rework removed `smoke-failure-reporter`, the last thing exercising `create-github-issue` end to end. Adds five deterministic Stage 3 scenarios: * `create-github-issue` — title-prefix application, body + `<!-- ado-aw -->` footer, and static/agent label merging * `create-github-issue-label-denied` — default-deny `allowed-labels` rejection via `expectedFailure` * `set-github-issue-type` — named type on an existing issue * `set-github-issue-type-clear` — the documented `issue_type: ""` clear * `create-github-issue-temporary-id-handoff` — the same-run `temporary_id` handoff, the highest-value case and the one with the weakest prior proof Harness changes: * new optional `Scenario.priorEntries` hook, so one `ado-aw execute` can process multiple NDJSON lines. The temporary-id registry (`ExecutionContext::resolved_github_issues`) is an in-process `Arc<Mutex<HashMap<_>>>`, so the handoff is only observable inside a single executor process — which is also exactly how production runs it. The runner reports a failed prior entry as its own execute-phase failure so a broken prerequisite can't look like an assertion failure. * `renderSourceMarkdown` now emits one `safe-outputs` key per tool * `assert()` gains a `records` argument exposing every executed record * GitHub REST plumbing extracted from the harness's failure reporter into a shared `github-client.ts`, so scenarios reuse it instead of adding a fourth GitHub client to the repo Cleanup closes rather than deletes, because GitHub has no delete-issue API. Titles embed the `ado-aw-det-*` marker, and cleanup falls back to a title search when a run fails before `assert()` populated the number, so an issue created by a non-succeeded record is still closed. Every missing precondition skips rather than fails: no token, no scratch repo, a token that can't write issues, or no org issue types. Issue types are organisation-level with no user-account equivalent, so the named-type scenarios skip on a user-owned scratch repo; the handoff stays runnable by falling back to the clear operation. The handoff assertion is mutation-checked — mismatched issue number, mismatched repository, a wrong echoed temporary id, and a fabricated result that GitHub does not corroborate all fail the test. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: cefea537-5177-4dfd-8849-bcc2caff2845
|
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. |
An earlier draft claimed there was no compile-time guard against putting `require-approval` on only one of the two GitHub issue tools. That is wrong: `validate_github_issue_outputs_config` in `src/compile/common.rs` rejects it, keyed on the *effective* setting so a section-level default and a per-tool override are both handled. Verified by compiling all three shapes (either direction, plus a global default with a per-tool opt-out) — each is refused. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: cefea537-5177-4dfd-8849-bcc2caff2845
Collaborator
Author
|
/review |
Contributor
|
🚀 review-typescript has started processing this pull request comment |
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.
I initially flagged this as an unguarded gap and was wrong.
require-approvalon only one of the two tools would split Stage 3 into twoexecuteprocesses and break the handoff — butvalidate_github_issue_outputs_configinsrc/compile/common.rsalready rejects that configuration, keyed on the effective setting so a section-level default and a per-tool override are both handled.Verified by compiling all three shapes (either direction, plus a global default with a per-tool opt-out); each is refused. The README note is corrected accordingly.
Testing -
npm run typecheck✅ -npx vitest run src/executor-e2e— 67 passed (was 29) ✅ -npm run build:executor-e2e✅ - Fullnpx vitest run— 890 passed, no regressions ✅ - Multi-key front matter, NDJSON ordering, record names and the label-rejection message were all verified against the realado-aw executebinary, not just mocks. Live proof comes from ADO definition2550, whose PR path filter already coversscripts/ado-script/src/executor-e2e/**andtests/executor-e2e/**.