diff --git a/scripts/ado-script/src/executor-e2e/__tests__/execute-cli.test.ts b/scripts/ado-script/src/executor-e2e/__tests__/execute-cli.test.ts index 3767c4af..c8170d69 100644 --- a/scripts/ado-script/src/executor-e2e/__tests__/execute-cli.test.ts +++ b/scripts/ado-script/src/executor-e2e/__tests__/execute-cli.test.ts @@ -10,7 +10,7 @@ describe("renderSourceMarkdown", () => { it("emits front matter with inline-JSON safe-outputs config", () => { const md = renderSourceMarkdown({ tool: "comment-on-work-item", - config: { target: "*", max: 1 }, + safeOutputs: { "comment-on-work-item": { target: "*", max: 1 } }, }); expect(md).toContain('name: "executor-e2e: comment-on-work-item"'); expect(md).toContain("target: standalone"); @@ -24,12 +24,25 @@ describe("renderSourceMarkdown", () => { it("emits a repos block when adoRepo is provided", () => { const md = renderSourceMarkdown({ tool: "add-pr-comment", - config: { "allowed-repositories": ["agent-definitions"] }, + safeOutputs: { "add-pr-comment": { "allowed-repositories": ["agent-definitions"] } }, adoRepo: "agent-definitions", }); expect(md).toContain("repos:"); expect(md).toContain(` - "agent-definitions=agent-definitions"`); }); + + it("emits one safe-outputs key per tool when a scenario stages prior entries", () => { + const md = renderSourceMarkdown({ + tool: "set-github-issue-type", + safeOutputs: { + "create-github-issue": { "target-repo": "o/r" }, + "set-github-issue-type": { "target-repo": "o/r" }, + }, + }); + expect(md).toContain('"create-github-issue": {"target-repo":"o/r"}'); + expect(md).toContain('"set-github-issue-type": {"target-repo":"o/r"}'); + expect(md.match(/^---$/gm)?.length).toBe(2); + }); }); describe("renderNdjsonLine", () => { diff --git a/scripts/ado-script/src/executor-e2e/__tests__/github-issue-scenarios.test.ts b/scripts/ado-script/src/executor-e2e/__tests__/github-issue-scenarios.test.ts new file mode 100644 index 00000000..e8332454 --- /dev/null +++ b/scripts/ado-script/src/executor-e2e/__tests__/github-issue-scenarios.test.ts @@ -0,0 +1,469 @@ +import { beforeEach, describe, expect, it, vi } from "vitest"; + +import type { ExecutedRecord, ScenarioContext } from "../scenario.js"; +import { SkipError } from "../scenario.js"; + +vi.mock("../github-client.js", async (importOriginal) => { + const actual = await importOriginal(); + return { + ...actual, + closeIssue: vi.fn(async () => {}), + createGitHubIssue: vi.fn(async () => "https://github.com/o/r/issues/123"), + diagnoseGitHubAuthFailure: vi.fn(async () => {}), + findOpenIssueByTitle: vi.fn(async () => undefined), + getIssue: vi.fn(async () => undefined), + listOrgIssueTypes: vi.fn(async () => []), + patchIssue: vi.fn(async () => ({ ok: false, status: 404, body: "" })), + }; +}); + +const gh = await import("../github-client.js"); +const { + createGithubIssue, + createGithubIssueLabelDenied, + createGithubIssueTemporaryIdHandoff, + githubIssueScenarios, + recordForTool, + resolveGithubIssueEnv, + setGithubIssueType, +} = await import("../scenarios/github-issue.js"); + +const TEMPORARY_ID = "#aw_e2e1"; +const REPO = "octo/scratch"; + +function fakeCtx(): ScenarioContext { + return { + orgUrl: "https://dev.azure.com/org/", + project: "P", + adoRepo: "agent-definitions", + buildId: "77", + token: "ado-token", + adoAwBin: "ado-aw", + workDir: "/tmp", + rest: {} as ScenarioContext["rest"], + log: () => {}, + prefix: (tool) => `ado-aw-det-77-${tool}`, + }; +} + +function record(name: string, result: Record): ExecutedRecord { + return { name, status: "succeeded", result }; +} + +/** Records for a healthy handoff: create filed #501, set-type resolved #501. */ +function handoffRecords(overrides: Record = {}): ExecutedRecord[] { + return [ + record("create_github_issue", { + number: 501, + url: "https://github.com/octo/scratch/issues/501", + target_repo: REPO, + temporary_id: TEMPORARY_ID, + }), + record("set_github_issue_type", { + number: 501, + target_repo: REPO, + issue_type: "", + ...overrides, + }), + ]; +} + +const goodEnv = { + EXECUTOR_E2E_GITHUB_TOKEN: "tok", + EXECUTOR_E2E_ISSUE_REPO: REPO, +} as NodeJS.ProcessEnv; + +beforeEach(() => { + vi.mocked(gh.getIssue).mockReset(); + vi.mocked(gh.closeIssue).mockReset(); + vi.mocked(gh.findOpenIssueByTitle).mockReset(); + vi.mocked(gh.patchIssue).mockReset(); + vi.mocked(gh.listOrgIssueTypes).mockReset(); + vi.mocked(gh.createGitHubIssue).mockReset(); + vi.mocked(gh.getIssue).mockResolvedValue(undefined); + vi.mocked(gh.closeIssue).mockResolvedValue(undefined); + vi.mocked(gh.findOpenIssueByTitle).mockResolvedValue(undefined); + vi.mocked(gh.patchIssue).mockResolvedValue({ ok: false, status: 404, body: "" }); + vi.mocked(gh.listOrgIssueTypes).mockResolvedValue([]); + vi.mocked(gh.createGitHubIssue).mockResolvedValue("https://github.com/o/r/issues/123"); +}); + +describe("resolveGithubIssueEnv", () => { + it("prefers the dedicated scenario repo over the failure-issue repo", () => { + const env = resolveGithubIssueEnv("t", { + EXECUTOR_E2E_GITHUB_TOKEN: "tok", + EXECUTOR_E2E_SCENARIO_ISSUE_REPO: "octo/scenarios", + EXECUTOR_E2E_ISSUE_REPO: "octo/failures", + } as NodeJS.ProcessEnv); + expect(env.repo).toBe("octo/scenarios"); + }); + + it("falls back to the failure-issue repo", () => { + expect(resolveGithubIssueEnv("t", goodEnv).repo).toBe(REPO); + }); + + it("skips when the token is missing", () => { + expect(() => + resolveGithubIssueEnv("t", { EXECUTOR_E2E_ISSUE_REPO: REPO } as NodeJS.ProcessEnv), + ).toThrow(SkipError); + }); + + it("treats an unexpanded ADO macro token as unset", () => { + expect(() => + resolveGithubIssueEnv("t", { + EXECUTOR_E2E_GITHUB_TOKEN: "$(EXECUTOR_E2E_GITHUB_TOKEN)", + EXECUTOR_E2E_ISSUE_REPO: REPO, + } as NodeJS.ProcessEnv), + ).toThrow(SkipError); + }); + + it("skips rather than defaulting to a canonical repo when none is configured", () => { + let thrown: unknown; + try { + resolveGithubIssueEnv("t", { EXECUTOR_E2E_GITHUB_TOKEN: "tok" } as NodeJS.ProcessEnv); + } catch (err) { + thrown = err; + } + expect(thrown).toBeInstanceOf(SkipError); + expect((thrown as Error).message).not.toContain("githubnext/ado-aw"); + }); + + it("treats an unexpanded repo macro as unset", () => { + expect(() => + resolveGithubIssueEnv("t", { + EXECUTOR_E2E_GITHUB_TOKEN: "tok", + EXECUTOR_E2E_ISSUE_REPO: "$(EXECUTOR_E2E_ISSUE_REPO)", + } as NodeJS.ProcessEnv), + ).toThrow(SkipError); + }); +}); + +describe("registry", () => { + it("registers five GitHub issue scenarios with unique ids", () => { + const ids = githubIssueScenarios.map((s) => s.id ?? s.tool); + expect(new Set(ids).size).toBe(5); + expect(ids).toEqual([ + "create-github-issue", + "create-github-issue-label-denied", + "set-github-issue-type", + "set-github-issue-type-clear", + "create-github-issue-temporary-id-handoff", + ]); + }); + + it("passes the harness token to the executor as ADO_AW_GITHUB_TOKEN", async () => { + const state = { repo: REPO, token: "tok", gh: { token: "tok", repo: REPO }, title: "t" }; + const env = await createGithubIssue.env!(fakeCtx(), state); + expect(env).toEqual({ ADO_AW_GITHUB_TOKEN: "tok" }); + }); + + it("targets the configured repo explicitly rather than relying on resolution", () => { + const state = { repo: REPO, token: "tok", gh: { token: "tok", repo: REPO }, title: "t" }; + for (const scenario of githubIssueScenarios) { + const config = scenario.config(fakeCtx(), state as never); + expect(config["target-repo"]).toBe(REPO); + } + }); +}); + +describe("create-github-issue", () => { + const state = () => ({ + repo: REPO, + token: "tok", + gh: { token: "tok", repo: REPO }, + title: "ado-aw-det-77-create-github-issue scratch issue", + issueNumber: undefined as number | undefined, + }); + + it("applies the title prefix and merges static + allowed agent labels", () => { + const config = createGithubIssue.config(fakeCtx(), state()); + expect(config["title-prefix"]).toBe("[executor-e2e] "); + expect(config.labels).toEqual(["executor-e2e"]); + expect(config["allowed-labels"]).toEqual(["executor-e2e-*"]); + }); + + it("asserts the prefixed title, footer marker and merged labels", async () => { + const s = state(); + vi.mocked(gh.getIssue).mockResolvedValue({ + number: 123, + title: `[executor-e2e] ${s.title}`, + body: `Deterministic executor E2E exercising create-github-issue for build 77. Safe to delete.\n\n`, + state: "open", + labels: ["executor-e2e", "executor-e2e-agent"], + }); + await expect( + createGithubIssue.assert( + fakeCtx(), + s, + record("create_github_issue", { number: 123, target_repo: REPO }), + [], + ), + ).resolves.toBeUndefined(); + expect(s.issueNumber).toBe(123); + }); + + it("fails when the executor drops the title prefix", async () => { + const s = state(); + vi.mocked(gh.getIssue).mockResolvedValue({ + number: 123, + title: s.title, + body: "Deterministic executor E2E exercising create-github-issue for build 77. Safe to delete.\n\n", + state: "open", + labels: ["executor-e2e", "executor-e2e-agent"], + }); + await expect( + createGithubIssue.assert( + fakeCtx(), + s, + record("create_github_issue", { number: 123, target_repo: REPO }), + [], + ), + ).rejects.toThrow(/issue title is/); + }); + + it("fails when a merged label is missing", async () => { + const s = state(); + vi.mocked(gh.getIssue).mockResolvedValue({ + number: 123, + title: `[executor-e2e] ${s.title}`, + body: "Deterministic executor E2E exercising create-github-issue for build 77. Safe to delete.\n\n", + state: "open", + labels: ["executor-e2e"], + }); + await expect( + createGithubIssue.assert( + fakeCtx(), + s, + record("create_github_issue", { number: 123, target_repo: REPO }), + [], + ), + ).rejects.toThrow(/missing 'executor-e2e-agent'/); + }); + + it("records the issue number before any fallible assertion so cleanup can close it", async () => { + const s = state(); + // No issue returned -> the very next check throws, but the number must + // already be captured for cleanup. + vi.mocked(gh.getIssue).mockResolvedValue(undefined); + await expect( + createGithubIssue.assert( + fakeCtx(), + s, + record("create_github_issue", { number: 456, target_repo: REPO }), + [], + ), + ).rejects.toThrow(/was not created/); + expect(s.issueNumber).toBe(456); + }); + + it("closes the issue it created during cleanup", async () => { + const s = { ...state(), issueNumber: 99 }; + await createGithubIssue.cleanup(fakeCtx(), s); + expect(gh.closeIssue).toHaveBeenCalledWith(s.gh, 99); + expect(gh.findOpenIssueByTitle).not.toHaveBeenCalled(); + }); + + it("falls back to the title marker when assert() never populated the number", async () => { + const s = state(); + vi.mocked(gh.findOpenIssueByTitle).mockResolvedValue(321); + await createGithubIssue.cleanup(fakeCtx(), s); + expect(gh.findOpenIssueByTitle).toHaveBeenCalledWith( + s.gh, + `[executor-e2e] ado-aw-det-77-create-github-issue scratch issue`, + ); + expect(gh.closeIssue).toHaveBeenCalledWith(s.gh, 321); + }); + + it("closes nothing when no marker-titled issue exists", async () => { + await createGithubIssue.cleanup(fakeCtx(), state()); + expect(gh.closeIssue).not.toHaveBeenCalled(); + }); +}); + +describe("create-github-issue allowed-labels rejection", () => { + it("expects a default-deny rejection rather than a success", () => { + expect(createGithubIssueLabelDenied.expectedFailure?.error.test( + "Agent-supplied labels not in allowed-labels: definitely-not-allowed", + )).toBe(true); + }); + + it("does NOT accept the 'no allowed-labels configured' message", () => { + // That message means the executor never read the operator config, so + // accepting it would make this scenario pass whether or not the + // allowlist actually took effect. + expect(createGithubIssueLabelDenied.expectedFailure?.error.test( + 'Agent-supplied labels rejected (no `allowed-labels` configured; set `allowed-labels: ["*"]` to permit any): x', + )).toBe(false); + }); + + it("proposes a label outside the allowlist", async () => { + const s = { repo: REPO, token: "tok", gh: { token: "tok", repo: REPO }, title: "t" }; + const entry = await createGithubIssueLabelDenied.ndjson(fakeCtx(), s); + expect(entry.labels).toEqual(["definitely-not-allowed"]); + const config = createGithubIssueLabelDenied.config(fakeCtx(), s); + expect(config["allowed-labels"]).toEqual(["executor-e2e-*"]); + }); +}); + +describe("set-github-issue-type", () => { + it("skips when the owner exposes no named issue types", async () => { + vi.stubEnv("EXECUTOR_E2E_GITHUB_TOKEN", "tok"); + vi.stubEnv("EXECUTOR_E2E_ISSUE_REPO", REPO); + vi.stubEnv("EXECUTOR_E2E_SCENARIO_ISSUE_REPO", ""); + vi.stubEnv("E2E_GITHUB_ISSUE_TYPE", ""); + vi.mocked(gh.listOrgIssueTypes).mockResolvedValue([]); + await expect(setGithubIssueType.setup(fakeCtx())).rejects.toThrow(SkipError); + // Nothing was created, so nothing can leak. + expect(gh.createGitHubIssue).not.toHaveBeenCalled(); + vi.unstubAllEnvs(); + }); + + it("fails when the executor targets a different issue", async () => { + const s = { + repo: REPO, + token: "tok", + gh: { token: "tok", repo: REPO }, + title: "t", + issueNumber: 10, + issueType: "Bug", + }; + await expect( + setGithubIssueType.assert( + fakeCtx(), + s, + record("set_github_issue_type", { number: 11, target_repo: REPO, issue_type: "Bug" }), + [], + ), + ).rejects.toThrow(/targeted issue #11, expected #10/); + }); +}); + +describe("temporary-ID handoff", () => { + const state = () => ({ + repo: REPO, + token: "tok", + gh: { token: "tok", repo: REPO }, + title: "ado-aw-det-77-create-github-issue-temporary-id-handoff scratch issue", + issueType: "", + issueNumber: undefined as number | undefined, + }); + + function issueMatching(s: ReturnType, type?: string) { + return { number: 501, title: s.title, body: "b", state: "open", labels: [], type }; + } + + it("stages create-github-issue ahead of the primary set-github-issue-type entry", async () => { + const s = state(); + const prior = await createGithubIssueTemporaryIdHandoff.priorEntries!(fakeCtx(), s); + expect(prior).toHaveLength(1); + expect(prior[0]!.tool).toBe("create-github-issue"); + expect(prior[0]!.entry.temporary_id).toBe(TEMPORARY_ID); + // require-temporary-id proves the producer is exercised on its strict path. + expect(prior[0]!.config["require-temporary-id"]).toBe(true); + + // The primary entry CONSUMES the temporary id. + expect(createGithubIssueTemporaryIdHandoff.tool).toBe("set-github-issue-type"); + const entry = await createGithubIssueTemporaryIdHandoff.ndjson(fakeCtx(), s); + expect(entry.issue_number).toBe(TEMPORARY_ID); + }); + + it("passes when the temporary id resolves to the issue that was actually filed", async () => { + const s = state(); + vi.mocked(gh.getIssue).mockResolvedValue(issueMatching(s)); + await expect( + createGithubIssueTemporaryIdHandoff.assert( + fakeCtx(), + s, + handoffRecords()[1]!, + handoffRecords(), + ), + ).resolves.toBeUndefined(); + expect(s.issueNumber).toBe(501); + }); + + // ---- MUTATION CHECKS ----------------------------------------------------- + // These deliberately break the handoff. If the assertion were vacuous (e.g. + // it only checked that the record existed) these would pass, which is exactly + // the failure mode this scenario is meant to rule out. + + it("MUTATION: fails when the resolved issue number does not match the filed one", async () => { + const s = state(); + vi.mocked(gh.getIssue).mockResolvedValue(issueMatching(s)); + const records = handoffRecords({ number: 999 }); + await expect( + createGithubIssueTemporaryIdHandoff.assert(fakeCtx(), s, records[1]!, records), + ).rejects.toThrow(/resolved to issue #999, but create-github-issue filed #501/); + }); + + it("MUTATION: fails when the resolved repository does not match the filed one", async () => { + const s = state(); + vi.mocked(gh.getIssue).mockResolvedValue(issueMatching(s)); + const records = handoffRecords({ target_repo: "someone/else" }); + await expect( + createGithubIssueTemporaryIdHandoff.assert(fakeCtx(), s, records[1]!, records), + ).rejects.toThrow(/resolved to repository 'someone\/else'/); + }); + + it("MUTATION: fails when the producer echoes a different temporary id", async () => { + const s = state(); + vi.mocked(gh.getIssue).mockResolvedValue(issueMatching(s)); + const records = handoffRecords(); + records[0]!.result!.temporary_id = "#aw_other"; + await expect( + createGithubIssueTemporaryIdHandoff.assert(fakeCtx(), s, records[1]!, records), + ).rejects.toThrow(/reported temporary_id '#aw_other'/); + }); + + it("MUTATION: fails when GitHub has no such issue, so a fabricated result cannot pass", async () => { + const s = state(); + vi.mocked(gh.getIssue).mockResolvedValue(undefined); + const records = handoffRecords(); + await expect( + createGithubIssueTemporaryIdHandoff.assert(fakeCtx(), s, records[1]!, records), + ).rejects.toThrow(/does not exist on/); + }); + + it("MUTATION: fails when the named type was not applied", async () => { + const s = { ...state(), issueType: "Bug" }; + vi.mocked(gh.getIssue).mockResolvedValue(issueMatching(s, undefined)); + const records = handoffRecords({ issue_type: "Bug" }); + await expect( + createGithubIssueTemporaryIdHandoff.assert(fakeCtx(), s, records[1]!, records), + ).rejects.toThrow(/has type '\(none\)', expected 'Bug'/); + }); + + it("captures the filed issue number before any fallible check", async () => { + const s = state(); + vi.mocked(gh.getIssue).mockResolvedValue(undefined); + const records = handoffRecords({ number: 999 }); + await expect( + createGithubIssueTemporaryIdHandoff.assert(fakeCtx(), s, records[1]!, records), + ).rejects.toThrow(); + expect(s.issueNumber).toBe(501); + }); + + it("closes the filed issue during cleanup", async () => { + const s = { ...state(), issueNumber: 501 }; + await createGithubIssueTemporaryIdHandoff.cleanup(fakeCtx(), s); + expect(gh.closeIssue).toHaveBeenCalledWith(s.gh, 501); + }); + + it("falls back to the title marker when the run failed before assert()", async () => { + const s = state(); + vi.mocked(gh.findOpenIssueByTitle).mockResolvedValue(777); + await createGithubIssueTemporaryIdHandoff.cleanup(fakeCtx(), s); + expect(gh.findOpenIssueByTitle).toHaveBeenCalledWith(s.gh, s.title); + expect(gh.closeIssue).toHaveBeenCalledWith(s.gh, 777); + }); +}); + +describe("recordForTool", () => { + it("maps kebab-case tool names onto snake_case record names", () => { + const records = handoffRecords(); + expect(recordForTool(records, "create-github-issue").result!.number).toBe(501); + }); + + it("throws when the record is absent", () => { + expect(() => recordForTool([], "create-github-issue")).toThrow(/no executed record/); + }); +}); diff --git a/scripts/ado-script/src/executor-e2e/__tests__/index.test.ts b/scripts/ado-script/src/executor-e2e/__tests__/index.test.ts index 91a19121..63291871 100644 --- a/scripts/ado-script/src/executor-e2e/__tests__/index.test.ts +++ b/scripts/ado-script/src/executor-e2e/__tests__/index.test.ts @@ -26,4 +26,18 @@ describe("scenario registry", () => { expect(ids).toContain("create-pull-request"); expect(ids).toContain("create-pull-request-self-multi-checkout"); }); + + it("registers the GitHub issue scenarios with unique ids", () => { + const ids = allScenarios.map((scenario) => scenario.id ?? scenario.tool); + expect(new Set(ids).size).toBe(ids.length); + for (const id of [ + "create-github-issue", + "create-github-issue-label-denied", + "set-github-issue-type", + "set-github-issue-type-clear", + "create-github-issue-temporary-id-handoff", + ]) { + expect(ids).toContain(id); + } + }); }); diff --git a/scripts/ado-script/src/executor-e2e/__tests__/runner.test.ts b/scripts/ado-script/src/executor-e2e/__tests__/runner.test.ts index bae31b31..1ceddd33 100644 --- a/scripts/ado-script/src/executor-e2e/__tests__/runner.test.ts +++ b/scripts/ado-script/src/executor-e2e/__tests__/runner.test.ts @@ -1,4 +1,4 @@ -import { mkdtemp, rm, writeFile } from "node:fs/promises"; +import { mkdtemp, readFile, rm, writeFile } from "node:fs/promises"; import { tmpdir } from "node:os"; import { join } from "node:path"; @@ -6,7 +6,7 @@ import { describe, expect, it } from "vitest"; import { runScenario } from "../runner.js"; import { SkipError } from "../scenario.js"; -import type { Scenario, ScenarioContext } from "../scenario.js"; +import type { ExecutedRecord, Scenario, ScenarioContext } from "../scenario.js"; function fakeCtx(): ScenarioContext { return { @@ -129,3 +129,144 @@ fs.writeFileSync(path.join(out, "safe-outputs-executed.ndjson"), JSON.stringify( } }); }); + +/** + * `priorEntries` lets one scenario stage extra safe-output lines ahead of its + * primary entry inside a single `ado-aw execute` run. These tests use a fake + * binary that echoes the staged NDJSON back as executed records, so they + * exercise the real staging/ordering/validation path without a real executor. + */ +describe("runScenario prior entries", () => { + /** + * Fake `ado-aw` that turns every staged input line into an executed record, + * preserving order. `statuses` overrides the status for a given tool. + */ + async function writeEchoBin(dir: string, statuses: Record = {}): Promise { + const bin = join(dir, "echo-ado-aw.js"); + await writeFile( + bin, + `#!/usr/bin/env node +const fs = require("node:fs"); +const path = require("node:path"); +const out = process.argv[process.argv.indexOf("--safe-output-dir") + 1]; +const statuses = ${JSON.stringify(statuses)}; +const lines = fs.readFileSync(path.join(out, "safe_outputs.ndjson"), "utf8") + .split(/\\r?\\n/).filter((l) => l.trim()); +const records = lines.map((l, i) => { + const parsed = JSON.parse(l); + return { + name: parsed.name.replaceAll("-", "_"), + status: statuses[parsed.name] ?? "succeeded", + error: statuses[parsed.name] ? "synthetic prior failure" : null, + result: { order: i, tool: parsed.name }, + }; +}); +fs.writeFileSync( + path.join(out, "safe-outputs-executed.ndjson"), + records.map((r) => JSON.stringify(r)).join("\\n") + "\\n", +); +`, + { encoding: "utf8", mode: 0o755 }, + ); + return bin; + } + + function handoffScenario( + onAssert: (records: ExecutedRecord[]) => void, + ): Scenario { + return { + id: "prior-entry-handoff", + tool: "set-github-issue-type", + config: () => ({ "target-repo": "o/r" }), + setup: async () => ({}), + priorEntries: async () => [ + { tool: "create-github-issue", config: { "target-repo": "o/r" }, entry: { title: "t" } }, + ], + ndjson: async () => ({ issue_number: "#aw_x1" }), + assert: async (_ctx, _state, _record, records) => onAssert(records), + cleanup: async () => {}, + }; + } + + it("writes prior entries before the primary entry and exposes all records to assert", async () => { + const dir = await mkdtemp(join(tmpdir(), "ado-aw-runner-prior-")); + try { + const bin = await writeEchoBin(dir); + let seen: ExecutedRecord[] = []; + const res = await runScenario( + { ...fakeCtx(), adoAwBin: bin, workDir: dir }, + handoffScenario((records) => { + seen = records; + }), + ); + expect(res.ok).toBe(true); + // Ordering matters: the producer must execute first, otherwise the + // in-process temporary-id registry has nothing to resolve. + expect(seen.map((r) => r.name)).toEqual([ + "create_github_issue", + "set_github_issue_type", + ]); + expect(seen[0]!.result!.order).toBe(0); + expect(seen[1]!.result!.order).toBe(1); + + // Both tools must appear in the rendered front matter, or the executor + // would report "not configured for this workflow". + const source = await readFile(join(dir, "prior-entry-handoff", "source.md"), "utf8"); + expect(source).toContain('"create-github-issue"'); + expect(source).toContain('"set-github-issue-type"'); + } finally { + await rm(dir, { recursive: true, force: true }); + } + }); + + it("fails in the execute phase when a prior entry did not succeed", async () => { + const dir = await mkdtemp(join(tmpdir(), "ado-aw-runner-prior-fail-")); + try { + const bin = await writeEchoBin(dir, { "create-github-issue": "failed" }); + let asserted = false; + const res = await runScenario( + { ...fakeCtx(), adoAwBin: bin, workDir: dir }, + handoffScenario(() => { + asserted = true; + }), + ); + expect(res.ok).toBe(false); + expect(res.phase).toBe("execute"); + expect(res.message).toContain("prior entry 'create-github-issue'"); + // The prerequisite failure must not be reported as an assertion failure. + expect(asserted).toBe(false); + } finally { + await rm(dir, { recursive: true, force: true }); + } + }); + + it("fails in the execute phase when a prior entry produced no record", async () => { + const dir = await mkdtemp(join(tmpdir(), "ado-aw-runner-prior-missing-")); + try { + const bin = join(dir, "drop-prior.js"); + await writeFile( + bin, + `#!/usr/bin/env node +const fs = require("node:fs"); +const path = require("node:path"); +const out = process.argv[process.argv.indexOf("--safe-output-dir") + 1]; +fs.writeFileSync(path.join(out, "safe-outputs-executed.ndjson"), JSON.stringify({ + name: "set_github_issue_type", + status: "succeeded", + result: {}, +}) + "\\n"); +`, + { encoding: "utf8", mode: 0o755 }, + ); + const res = await runScenario( + { ...fakeCtx(), adoAwBin: bin, workDir: dir }, + handoffScenario(() => {}), + ); + expect(res.ok).toBe(false); + expect(res.phase).toBe("execute"); + expect(res.message).toContain("produced no executed record"); + } finally { + await rm(dir, { recursive: true, force: true }); + } + }); +}); diff --git a/scripts/ado-script/src/executor-e2e/execute-cli.ts b/scripts/ado-script/src/executor-e2e/execute-cli.ts index 630bfffb..e4db83a5 100644 --- a/scripts/ado-script/src/executor-e2e/execute-cli.ts +++ b/scripts/ado-script/src/executor-e2e/execute-cli.ts @@ -16,15 +16,15 @@ import { mkdir, readFile, writeFile } from "node:fs/promises"; import { existsSync } from "node:fs"; import { join, resolve, sep } from "node:path"; -import type { ExecutedRecord } from "./scenario.js"; +import type { ExecutedRecord, PriorEntry } from "./scenario.js"; const SAFE_OUTPUT_FILENAME = "safe_outputs.ndjson"; const EXECUTED_FILENAME = "safe-outputs-executed.ndjson"; export interface RenderSourceOptions { tool: string; - /** Per-tool `safe-outputs: :` config object. */ - config: Record; + /** Per-tool `safe-outputs: :` config map (one key per tool used). */ + safeOutputs: Record>; /** ADO repo name for repo-targeting tools (emits a `repos:` block). */ adoRepo?: string; } @@ -53,9 +53,12 @@ export function renderSourceMarkdown(opts: RenderSourceOptions): string { lines.push(` - ${JSON.stringify(`${opts.adoRepo}=${opts.adoRepo}`)}`); } lines.push("safe-outputs:"); - // Quote the tool key (JSON string is valid YAML) so an unusual tool name - // containing ": " or a leading "{" can't emit broken YAML. - lines.push(` ${JSON.stringify(opts.tool)}: ${JSON.stringify(opts.config)}`); + // Quote every tool key (a JSON string is valid YAML) so an unusual tool name + // containing ": " or a leading "{" can't emit broken YAML. A scenario using + // `priorEntries` contributes more than one key here. + for (const [tool, config] of Object.entries(opts.safeOutputs)) { + lines.push(` ${JSON.stringify(tool)}: ${JSON.stringify(config)}`); + } lines.push("---"); lines.push(""); lines.push(`Deterministic executor E2E fixture for \`${opts.tool}\`.`); @@ -77,6 +80,12 @@ export interface RunExecuteOptions { tool: string; config: Record; entry: Record; + /** + * Extra entries written to the NDJSON **before** `entry`, executed by the + * same `ado-aw execute` process in the order given. Their configs are merged + * into the rendered `safe-outputs:` block. See `PriorEntry` in `scenario.ts`. + */ + priorEntries?: PriorEntry[]; adoRepo?: string; orgUrl: string; project: string; @@ -120,9 +129,15 @@ export async function runExecute(opts: RunExecuteOptions): Promise> = {}; + for (const prior of priorEntries) safeOutputs[prior.tool] = prior.config; + safeOutputs[opts.tool] = opts.config; await writeFile( sourcePath, - renderSourceMarkdown({ tool: opts.tool, config: opts.config, adoRepo: opts.adoRepo }), + renderSourceMarkdown({ tool: opts.tool, safeOutputs, adoRepo: opts.adoRepo }), "utf8", ); @@ -139,11 +154,14 @@ export async function runExecute(opts: RunExecuteOptions): Promise renderNdjsonLine(prior.tool, prior.entry)), renderNdjsonLine(opts.tool, opts.entry), - "utf8", - ); + ].join(""); + await writeFile(join(safeOutputDir, SAFE_OUTPUT_FILENAME), ndjson, "utf8"); const args = [ "execute", diff --git a/scripts/ado-script/src/executor-e2e/github-client.ts b/scripts/ado-script/src/executor-e2e/github-client.ts new file mode 100644 index 00000000..d0054044 --- /dev/null +++ b/scripts/ado-script/src/executor-e2e/github-client.ts @@ -0,0 +1,285 @@ +/** + * The one GitHub REST client used by the executor E2E harness. + * + * Three GitHub API clients exist in this repository, and they are deliberately + * separate because they sit on different sides of a trust or language boundary: + * + * 1. `src/safe_outputs/create_github_issue.rs` + `set_github_issue_type.rs` + * (Rust / `reqwest`) — the shipped executor, i.e. the code under test. + * 2. `scripts/ado-script/src/github-app-token/index.ts` — a shipped bundle that + * mints a GitHub App installation token inside the Agent/Detection jobs. + * 3. **This module** — test-harness only, used by both the harness's own + * failure reporter (`github-issue.ts`) and the GitHub issue scenarios + * (`scenarios/github-issue.ts`). + * + * Scenario code must reuse this module rather than adding a fourth ad-hoc + * `fetch` wrapper. + * + * Test-harness module; not shipped in `ado-script.zip`. + */ + +export type FetchImpl = typeof fetch; + +/** Default per-request timeout for GitHub API calls, matching AdoRest's 30s. */ +export const DEFAULT_GITHUB_TIMEOUT_MS = 30_000; + +export interface GitHubClientOptions { + token: string; + /** `owner/repo` slug. */ + repo: string; + fetchImpl?: FetchImpl; + /** Per-request timeout in ms (defaults to DEFAULT_GITHUB_TIMEOUT_MS). */ + timeoutMs?: number; +} + +export function ghHeaders(token: string): Record { + return { + Authorization: `Bearer ${token}`, + Accept: "application/vnd.github+json", + "X-GitHub-Api-Version": "2022-11-28", + "User-Agent": "ado-aw-executor-e2e", + }; +} + +function ghFetch(opts: GitHubClientOptions): FetchImpl { + return opts.fetchImpl ?? fetch; +} + +function ghSignal(opts: GitHubClientOptions): AbortSignal { + // Bound every GitHub call so a hung response can't stall the harness + // indefinitely and burn the ADO job's wall-clock limit. + return AbortSignal.timeout(opts.timeoutMs ?? DEFAULT_GITHUB_TIMEOUT_MS); +} + +/** + * Trim a pipeline env value, treating an UNEXPANDED ADO macro (e.g. the literal + * `$(EXECUTOR_E2E_ISSUE_REPO)`) as absent. ADO passes a `$(VAR)` reference + * through verbatim when VAR is undefined, so without this guard an unset + * override would be used as a bogus repo slug instead of falling back to the + * default. + */ +export function cleanVar(raw: string | undefined): string | undefined { + const value = raw?.trim(); + if (!value || /^\$\(.*\)$/.test(value)) return undefined; + return value; +} + +/** Split an `owner/repo` slug; throws when it is not in that form. */ +export function splitRepo(repo: string): { owner: string; name: string } { + const [owner, name, ...rest] = repo.split("/"); + if (!owner || !name || rest.length > 0) { + throw new Error(`expected an 'owner/repo' slug, got '${repo}'`); + } + return { owner, name }; +} + +/** Return the number of an open issue with this exact title, if one exists. */ +export async function findOpenIssueByTitle( + opts: GitHubClientOptions, + title: string, +): Promise { + const q = `repo:${opts.repo} is:issue is:open in:title ${JSON.stringify(title)}`; + // GitHub search does partial-phrase matching, so many open issues can share + // the title's words. Page at 100 (scoped to repo + is:open + in:title, so + // this comfortably covers the expected scale) to avoid the exact-match + // .find() missing an existing issue and filing a duplicate. + const url = `https://api.github.com/search/issues?q=${encodeURIComponent(q)}&per_page=100`; + const res = await ghFetch(opts)(url, { + headers: ghHeaders(opts.token), + signal: ghSignal(opts), + }); + if (!res.ok) throw new Error(`GitHub search failed: HTTP ${res.status}`); + const json = (await res.json()) as { items?: { number: number; title: string }[] }; + return json.items?.find((i) => i.title === title)?.number; +} + +export async function createGitHubIssue( + opts: GitHubClientOptions, + title: string, + body: string, + labels: string[], +): Promise { + const url = `https://api.github.com/repos/${opts.repo}/issues`; + const res = await ghFetch(opts)(url, { + method: "POST", + headers: { ...ghHeaders(opts.token), "Content-Type": "application/json" }, + body: JSON.stringify({ title, body, labels }), + signal: ghSignal(opts), + }); + if (!res.ok) { + const text = await res.text().catch(() => ""); + throw new Error(`GitHub create issue failed: HTTP ${res.status}: ${text}`); + } + const json = (await res.json()) as { html_url?: string }; + return json.html_url ?? "(created)"; +} + +/** One GitHub issue, reduced to the fields the scenarios assert on. */ +export interface GitHubIssue { + number: number; + title: string; + body: string | null; + state: string; + labels: string[]; + /** Native issue type name, or undefined when the issue has no type. */ + type?: string; +} + +interface RawIssue { + number?: number; + title?: string; + body?: string | null; + state?: string; + labels?: (string | { name?: string })[]; + type?: { name?: string } | null; +} + +function toIssue(raw: RawIssue): GitHubIssue { + return { + number: typeof raw.number === "number" ? raw.number : 0, + title: raw.title ?? "", + body: raw.body ?? null, + state: raw.state ?? "", + labels: (raw.labels ?? []) + .map((l) => (typeof l === "string" ? l : (l.name ?? ""))) + .filter((l) => l.length > 0), + type: raw.type?.name ?? undefined, + }; +} + +/** Fetch a single issue. Returns undefined on 404. */ +export async function getIssue( + opts: GitHubClientOptions, + issueNumber: number, +): Promise { + const url = `https://api.github.com/repos/${opts.repo}/issues/${issueNumber}`; + const res = await ghFetch(opts)(url, { + headers: ghHeaders(opts.token), + signal: ghSignal(opts), + }); + if (res.status === 404) return undefined; + if (!res.ok) { + const text = await res.text().catch(() => ""); + throw new Error(`GitHub get issue #${issueNumber} failed: HTTP ${res.status}: ${text}`); + } + return toIssue((await res.json()) as RawIssue); +} + +/** + * PATCH an issue. Returns the HTTP status rather than throwing so callers can + * use it as a capability probe (e.g. "does this repo accept a type clear?"). + */ +export async function patchIssue( + opts: GitHubClientOptions, + issueNumber: number, + payload: Record, +): Promise<{ ok: boolean; status: number; body: string }> { + const url = `https://api.github.com/repos/${opts.repo}/issues/${issueNumber}`; + const res = await ghFetch(opts)(url, { + method: "PATCH", + headers: { ...ghHeaders(opts.token), "Content-Type": "application/json" }, + body: JSON.stringify(payload), + signal: ghSignal(opts), + }); + const body = await res.text().catch(() => ""); + return { ok: res.ok, status: res.status, body }; +} + +/** + * Close an issue as `not_planned`. + * + * GitHub has no REST endpoint to DELETE an issue, so "close" is the strongest + * teardown available to the harness — see the close-not-delete note in + * `tests/executor-e2e/README.md`. + */ +export async function closeIssue( + opts: GitHubClientOptions, + issueNumber: number, +): Promise { + const res = await patchIssue(opts, issueNumber, { + state: "closed", + state_reason: "not_planned", + }); + if (!res.ok) { + throw new Error(`GitHub close issue #${issueNumber} failed: HTTP ${res.status}: ${res.body}`); + } +} + +/** + * List an organisation's native issue types. + * + * Issue types are an **organisation-level** construct: there is no user-account + * equivalent of `GET /orgs/{org}/issue-types`. A user-owned repository + * therefore always yields an empty list, which callers must treat as "skip", + * not "fail". + */ +export async function listOrgIssueTypes( + opts: GitHubClientOptions, + owner: string, +): Promise { + const url = `https://api.github.com/orgs/${encodeURIComponent(owner)}/issue-types`; + const res = await ghFetch(opts)(url, { + headers: ghHeaders(opts.token), + signal: ghSignal(opts), + }); + // 404: not an org (or the feature is unavailable). 403: the token cannot read + // org metadata. Both mean "no discoverable named type", never a hard failure. + if (res.status === 404 || res.status === 403) return []; + if (!res.ok) { + throw new Error(`GitHub list issue types for '${owner}' failed: HTTP ${res.status}`); + } + const json = (await res.json()) as { name?: string }[] | { message?: string }; + if (!Array.isArray(json)) return []; + return json.map((t) => t.name ?? "").filter((n) => n.length > 0); +} + +/** + * On a GitHub auth/permission failure (401/403), probe `GET /user` to report + * exactly what went wrong instead of leaving the operator to guess. Turns an + * opaque "HTTP 403" into an actionable line naming the target repo, the + * authenticated login (or "token invalid/revoked" on 401), and the token's + * accepted permissions. Best-effort: never throws. + */ +export async function diagnoseGitHubAuthFailure( + opts: GitHubClientOptions, + status: number, + log: (msg: string) => void, +): Promise { + if (status !== 401 && status !== 403) return; + try { + const res = await ghFetch(opts)("https://api.github.com/user", { + headers: ghHeaders(opts.token), + signal: ghSignal(opts), + }); + const accepted = res.headers.get("x-accepted-github-permissions") ?? "(none reported)"; + if (res.status === 401) { + log( + `GitHub token diagnosis: HTTP 401 from /user — the token is invalid, expired, or REVOKED ` + + `(GitHub auto-revokes tokens shared in plaintext). Generate a fresh token. Target repo: ${opts.repo}.`, + ); + return; + } + if (res.ok) { + const user = (await res.json()) as { login?: string }; + log( + `GitHub token diagnosis: authenticated as '${user.login ?? "?"}' but got HTTP ${status} filing to ` + + `'${opts.repo}'. The token authenticates but lacks Issues:write on that repo (or, for a fine-grained ` + + `PAT, its resource-owner/repository-access does not include it). Accepted perms: ${accepted}.`, + ); + return; + } + log( + `GitHub token diagnosis: HTTP ${status} filing to '${opts.repo}'; /user probe returned ${res.status}. ` + + `Check the token's Issues:write permission and repository access.`, + ); + } catch (err) { + log(`GitHub token diagnosis probe failed: ${err instanceof Error ? err.message : String(err)}`); + } +} + +/** Extract a trailing "HTTP " code from a thrown GitHub client error. */ +export function statusFromError(err: unknown): number | undefined { + const message = err instanceof Error ? err.message : String(err); + const match = message.match(/HTTP (\d{3})/); + return match ? Number(match[1]) : undefined; +} diff --git a/scripts/ado-script/src/executor-e2e/github-issue.ts b/scripts/ado-script/src/executor-e2e/github-issue.ts index f36fb6fa..6311ed08 100644 --- a/scripts/ado-script/src/executor-e2e/github-issue.ts +++ b/scripts/ado-script/src/executor-e2e/github-issue.ts @@ -8,8 +8,20 @@ * * Test-harness module; not shipped in `ado-script.zip`. */ +import { + createGitHubIssue, + diagnoseGitHubAuthFailure, + findOpenIssueByTitle, + cleanVar, + statusFromError, +} from "./github-client.js"; +import type { FetchImpl, GitHubClientOptions } from "./github-client.js"; import type { ScenarioResult } from "./scenario.js"; +// Re-exported for the harness's own tests and for scenario modules that need +// the same primitives; there is exactly one GitHub client (github-client.ts). +export { createGitHubIssue, diagnoseGitHubAuthFailure, findOpenIssueByTitle }; + export const ISSUE_TITLE_PREFIX = "[executor-e2e-failure] "; const DEFAULT_REPO = "githubnext/ado-aw"; const DEFAULT_LABELS = ["executor-e2e-failure", "pipeline-failure"]; @@ -49,19 +61,6 @@ export function loadIssueEnv(env: NodeJS.ProcessEnv = process.env): IssueEnv { }; } -/** - * Trim a pipeline env value, treating an UNEXPANDED ADO macro (e.g. the literal - * `$(EXECUTOR_E2E_ISSUE_REPO)`) as absent. ADO passes a `$(VAR)` reference - * through verbatim when VAR is undefined, so without this guard an unset - * override would be used as a bogus repo slug instead of falling back to the - * default. - */ -function cleanVar(raw: string | undefined): string | undefined { - const value = raw?.trim(); - if (!value || /^\$\(.*\)$/.test(value)) return undefined; - return value; -} - /** * Build a stable issue title keyed on the sorted set of failing tools, so a * recurring failure signature dedupes to a single open issue. @@ -113,124 +112,12 @@ export function renderIssueBody( return lines.join("\n"); } -type FetchImpl = typeof fetch; - -/** Default per-request timeout for GitHub API calls, matching AdoRest's 30s. */ -const DEFAULT_GITHUB_TIMEOUT_MS = 30_000; - -interface GitHubClientOptions { - token: string; - repo: string; - fetchImpl?: FetchImpl; - /** Per-request timeout in ms (defaults to DEFAULT_GITHUB_TIMEOUT_MS). */ - timeoutMs?: number; -} - -function ghHeaders(token: string): Record { - return { - Authorization: `Bearer ${token}`, - Accept: "application/vnd.github+json", - "X-GitHub-Api-Version": "2022-11-28", - "User-Agent": "ado-aw-executor-e2e", - }; -} - -/** Return the number of an open issue with this exact title, if one exists. */ -export async function findOpenIssueByTitle( - opts: GitHubClientOptions, - title: string, -): Promise { - const fetchImpl = opts.fetchImpl ?? fetch; - const q = `repo:${opts.repo} is:issue is:open in:title ${JSON.stringify(title)}`; - // GitHub search does partial-phrase matching, so many open issues can share - // the title's words. Page at 100 (scoped to repo + is:open + in:title, so - // this comfortably covers the expected scale) to avoid the exact-match - // .find() missing an existing issue and filing a duplicate. - const url = `https://api.github.com/search/issues?q=${encodeURIComponent(q)}&per_page=100`; - const res = await fetchImpl(url, { - headers: ghHeaders(opts.token), - // Bound every GitHub call so a hung response can't stall main() indefinitely - // after all scenarios complete and burn the ADO job's wall-clock limit. - signal: AbortSignal.timeout(opts.timeoutMs ?? DEFAULT_GITHUB_TIMEOUT_MS), - }); - if (!res.ok) throw new Error(`GitHub search failed: HTTP ${res.status}`); - const json = (await res.json()) as { items?: { number: number; title: string }[] }; - return json.items?.find((i) => i.title === title)?.number; -} - -export async function createGitHubIssue( - opts: GitHubClientOptions, - title: string, - body: string, - labels: string[], -): Promise { - const fetchImpl = opts.fetchImpl ?? fetch; - const url = `https://api.github.com/repos/${opts.repo}/issues`; - const res = await fetchImpl(url, { - method: "POST", - headers: { ...ghHeaders(opts.token), "Content-Type": "application/json" }, - body: JSON.stringify({ title, body, labels }), - signal: AbortSignal.timeout(opts.timeoutMs ?? DEFAULT_GITHUB_TIMEOUT_MS), - }); - if (!res.ok) { - const text = await res.text().catch(() => ""); - throw new Error(`GitHub create issue failed: HTTP ${res.status}: ${text}`); - } - const json = (await res.json()) as { html_url?: string }; - return json.html_url ?? "(created)"; -} - export interface FileIssueOutcome { filed: boolean; reason?: string; url?: string; } -/** - * On a GitHub auth/permission failure (401/403), probe `GET /user` to report - * exactly what went wrong instead of leaving the operator to guess. Turns an - * opaque "HTTP 403" into an actionable line naming the target repo, the - * authenticated login (or "token invalid/revoked" on 401), and the token's - * accepted permissions. Best-effort: never throws. - */ -export async function diagnoseGitHubAuthFailure( - opts: GitHubClientOptions, - status: number, - log: (msg: string) => void, -): Promise { - if (status !== 401 && status !== 403) return; - const fetchImpl = opts.fetchImpl ?? fetch; - try { - const res = await fetchImpl("https://api.github.com/user", { - headers: ghHeaders(opts.token), - signal: AbortSignal.timeout(opts.timeoutMs ?? DEFAULT_GITHUB_TIMEOUT_MS), - }); - const accepted = res.headers.get("x-accepted-github-permissions") ?? "(none reported)"; - if (res.status === 401) { - log( - `GitHub token diagnosis: HTTP 401 from /user — the token is invalid, expired, or REVOKED ` + - `(GitHub auto-revokes tokens shared in plaintext). Generate a fresh token. Target repo: ${opts.repo}.`, - ); - return; - } - if (res.ok) { - const user = (await res.json()) as { login?: string }; - log( - `GitHub token diagnosis: authenticated as '${user.login ?? "?"}' but got HTTP ${status} filing to ` + - `'${opts.repo}'. The token authenticates but lacks Issues:write on that repo (or, for a fine-grained ` + - `PAT, its resource-owner/repository-access does not include it). Accepted perms: ${accepted}.`, - ); - return; - } - log( - `GitHub token diagnosis: HTTP ${status} filing to '${opts.repo}'; /user probe returned ${res.status}. ` + - `Check the token's Issues:write permission and repository access.`, - ); - } catch (err) { - log(`GitHub token diagnosis probe failed: ${err instanceof Error ? err.message : String(err)}`); - } -} - /** * File (or dedupe) a failure issue. No-op when there are no failures or when no * token is configured. @@ -270,10 +157,3 @@ export async function fileFailureIssue( throw err; } } - -/** Extract a trailing "HTTP " code from a thrown GitHub client error. */ -function statusFromError(err: unknown): number | undefined { - const message = err instanceof Error ? err.message : String(err); - const match = message.match(/HTTP (\d{3})/); - return match ? Number(match[1]) : undefined; -} diff --git a/scripts/ado-script/src/executor-e2e/runner.ts b/scripts/ado-script/src/executor-e2e/runner.ts index a6d958db..998c690b 100644 --- a/scripts/ado-script/src/executor-e2e/runner.ts +++ b/scripts/ado-script/src/executor-e2e/runner.ts @@ -67,10 +67,11 @@ export async function runScenario( // Guard the auxiliary scenario methods too: a harness-level bug in any of // these must record a failed result and let the rest of the suite run, // not propagate out of runScenario and abort runAll early. - let config, entry, files, extraEnv; + let config, entry, files, extraEnv, priorEntries; try { config = scenario.config(ctx, state); entry = await scenario.ndjson(ctx, state); + priorEntries = scenario.priorEntries ? await scenario.priorEntries(ctx, state) : undefined; files = scenario.files ? await scenario.files(ctx, state) : undefined; extraEnv = scenario.env ? await scenario.env(ctx, state) : undefined; } catch (err) { @@ -85,6 +86,7 @@ export async function runScenario( tool, config, entry, + priorEntries, adoRepo: scenario.targetsAdoRepo ? ctx.adoRepo : undefined, orgUrl: ctx.orgUrl, project: ctx.project, @@ -98,6 +100,27 @@ export async function runScenario( return finish({ ok: false, phase: "execute", message: errMessage(err) }); } + // Prior entries are prerequisites, not the thing under test: surface a + // broken one as its own execute-phase failure so it can never be mistaken + // for an assertion failure in the primary tool. + for (const prior of priorEntries ?? []) { + const priorRecord = result.records.find((r) => r.name === prior.tool.replaceAll("-", "_")); + if (!priorRecord) { + return finish({ + ok: false, + phase: "execute", + message: `prior entry '${prior.tool}' produced no executed record`, + }); + } + if (priorRecord.status !== "succeeded") { + return finish({ + ok: false, + phase: "execute", + message: `prior entry '${prior.tool}' reported status='${priorRecord.status}': ${priorRecord.error ?? "no error message"}`, + }); + } + } + if (!result.record) { return finish({ ok: false, @@ -125,7 +148,7 @@ export async function runScenario( // ---- assert ---- try { - await scenario.assert(ctx, state, result.record); + await scenario.assert(ctx, state, result.record, result.records); } catch (err) { return finish({ ok: false, phase: "assert", message: errMessage(err) }); } diff --git a/scripts/ado-script/src/executor-e2e/scenario.ts b/scripts/ado-script/src/executor-e2e/scenario.ts index 622a4fb1..b6b1341c 100644 --- a/scripts/ado-script/src/executor-e2e/scenario.ts +++ b/scripts/ado-script/src/executor-e2e/scenario.ts @@ -26,8 +26,31 @@ export interface ExecutedRecord { timestamp?: string; } -/** Shared, read-only context handed to every scenario phase. */ -export interface ScenarioContext { +/** + * One extra safe-output entry staged **before** a scenario's primary entry, in + * the same `ado-aw execute` invocation. + * + * This exists because some safe outputs hand state to each other through + * in-process state that never touches disk. `create-github-issue` registers a + * `temporary_id` in `ExecutionContext.resolved_github_issues` — an + * `Arc>>` — which `set-github-issue-type` then resolves. That + * handoff is only observable when both entries are lines in the same NDJSON + * processed by one executor process. + * + * That matches production: a SafeOutputs job runs a single `ado-aw execute` + * over the whole `safe_outputs.ndjson`, processing entries sequentially in file + * order. `priorEntries` reproduces exactly that shape. + */ +export interface PriorEntry { + /** kebab-case safe-output tool name for this line. */ + readonly tool: string; + /** NDJSON params WITHOUT the `name` field — the runner injects it. */ + readonly entry: Record; + /** `safe-outputs: :` config fragment for this line's tool. */ + readonly config: Record; +} + +/** Shared, read-only context handed to every scenario phase. */export interface ScenarioContext { /** ADO collection URI, e.g. https://dev.azure.com/msazuresphere/ */ readonly orgUrl: string; /** ADO project name, e.g. AgentPlayground. */ @@ -91,6 +114,19 @@ export interface Scenario { * injects `name: `). */ ndjson(ctx: ScenarioContext, state: State): Promise>; + /** + * Optional safe-output entries staged **before** `ndjson()`, executed by the + * same `ado-aw execute` process and in the returned order. + * + * Their `config` fragments are merged into the rendered `safe-outputs:` block + * alongside the primary tool's. The runner fails the scenario in the + * `execute` phase if any prior entry is missing from the executed NDJSON or + * did not report `succeeded`, so a broken prerequisite can never be mistaken + * for an assertion failure. + * + * See {@link PriorEntry} for why this mechanism exists. + */ + priorEntries?(ctx: ScenarioContext, state: State): Promise; /** * Optional extra files to stage into the safe-output dir before running the * executor (relative path -> UTF-8 contents). Used by attachment and @@ -113,11 +149,20 @@ export interface Scenario { /** * Assert the ADO side-effect actually happened. Throw on failure. * + * `record` is the executed record for this scenario's primary `tool`; + * `records` carries every parsed record from the run, which is how a scenario + * using {@link Scenario.priorEntries} reads results from its prior entries. + * * May populate fields on `state` (e.g. an id read from the executor result) * that `cleanup` needs — do this **before** any fallible check so cleanup can * still tear the object down if a later assertion throws. */ - assert(ctx: ScenarioContext, state: State, record: ExecutedRecord): Promise; + assert( + ctx: ScenarioContext, + state: State, + record: ExecutedRecord, + records: ExecutedRecord[], + ): Promise; /** Best-effort teardown of everything setup/execute created. */ cleanup(ctx: ScenarioContext, state: State): Promise; } diff --git a/scripts/ado-script/src/executor-e2e/scenarios/github-issue.ts b/scripts/ado-script/src/executor-e2e/scenarios/github-issue.ts new file mode 100644 index 00000000..62afa4e7 --- /dev/null +++ b/scripts/ado-script/src/executor-e2e/scenarios/github-issue.ts @@ -0,0 +1,560 @@ +/** + * GitHub issue safe-output scenarios: `create-github-issue`, + * `set-github-issue-type`, and the same-run `temporary_id` handoff between + * them. + * + * These are the only scenarios that assert against **GitHub** rather than ADO, + * so `ctx.rest` (an `AdoRest`) is unused here; they drive the shared harness + * GitHub client in `../github-client.js` instead. + * + * ## Close, don't delete + * + * Every other scenario in this suite tears down completely. These cannot: + * GitHub has no REST endpoint to delete an issue. `cleanup()` therefore + * **closes** each issue as `not_planned`, and every title embeds the greppable + * `ado-aw-det--` marker so anything a cleanup misses is findable + * with a single search. This is documented in `tests/executor-e2e/README.md`. + * + * ## Why so many skips + * + * Native issue types are an **organisation-level** construct — there is no + * user-account equivalent of `GET /orgs/{org}/issue-types` — so a user-owned + * scratch repository can never expose a named type. Scenarios that need one + * raise `SkipError`, never a failure. The handoff scenario stays runnable by + * falling back to the documented clear operation (`issue_type: ""`). + * + * Test-harness module; not shipped in `ado-script.zip`. + */ +import { + cleanVar, + closeIssue, + createGitHubIssue, + diagnoseGitHubAuthFailure, + findOpenIssueByTitle, + getIssue, + listOrgIssueTypes, + patchIssue, + splitRepo, +} from "../github-client.js"; +import type { GitHubClientOptions } from "../github-client.js"; +import type { ExecutedRecord, PriorEntry, Scenario, ScenarioContext } from "../scenario.js"; +import { SkipError } from "../scenario.js"; +import { detBody, numResult, strResult, Teardown } from "./common.js"; + +/** Static label the operator config always applies. */ +const STATIC_LABEL = "executor-e2e"; +/** Agent-supplied label that the `allowed-labels` pattern below admits. */ +const ALLOWED_AGENT_LABEL = "executor-e2e-agent"; +/** Agent-supplied label deliberately outside the allowlist. */ +const DENIED_AGENT_LABEL = "definitely-not-allowed"; +/** Wildcard pattern that admits `ALLOWED_AGENT_LABEL` but not `DENIED_AGENT_LABEL`. */ +const ALLOWED_LABEL_PATTERN = "executor-e2e-*"; +/** Prefix the operator config prepends to every agent-supplied title. */ +const TITLE_PREFIX = "[executor-e2e] "; +/** Temporary ID exercised by the handoff scenario. */ +const TEMPORARY_ID = "#aw_e2e1"; +/** Traceability marker the executor appends to every issue body. */ +const FOOTER_MARKER = ""; + +/** Everything the scenarios need after preconditions are resolved. */ +export interface GithubIssueEnv { + /** `owner/repo` slug that scratch issues are filed into. */ + repo: string; + token: string; + gh: GitHubClientOptions; +} + +/** + * Resolve the GitHub token and target repo, or `SkipError`. + * + * There is deliberately **no default repo**: an unset/misconfigured variable + * must skip rather than spray scratch issues onto the canonical repository. + */ +export function resolveGithubIssueEnv( + tool: string, + env: NodeJS.ProcessEnv = process.env, +): GithubIssueEnv { + const token = cleanVar(env.EXECUTOR_E2E_GITHUB_TOKEN); + if (!token) { + throw new SkipError( + `${tool}: EXECUTOR_E2E_GITHUB_TOKEN is not set; supply a PAT with Issues:write to enable this scenario`, + ); + } + const repo = cleanVar(env.EXECUTOR_E2E_SCENARIO_ISSUE_REPO) ?? cleanVar(env.EXECUTOR_E2E_ISSUE_REPO); + if (!repo) { + throw new SkipError( + `${tool}: no scratch issue repo configured (set EXECUTOR_E2E_SCENARIO_ISSUE_REPO or EXECUTOR_E2E_ISSUE_REPO)`, + ); + } + // Reject a malformed slug up front rather than letting it 404 mid-scenario. + splitRepo(repo); + return { repo, token, gh: { token, repo } }; +} + +/** + * Confirm the token can actually mutate issues on the target repo before the + * scenario creates anything. + * + * A token that authenticates but lacks Issues:write is a **missing + * precondition**, not a product failure, so this skips (with the harness's + * standard auth diagnosis attached) rather than going red. + */ +async function requireIssueWrite(env: GithubIssueEnv, tool: string): Promise { + const probe = await patchIssue(env.gh, 0, {}).catch(() => undefined); + // Issue #0 never exists; 404 means the token reached the repo, which is all + // we can check without mutating real state. 401/403 means it cannot write. + if (probe && (probe.status === 401 || probe.status === 403)) { + const diagnosis: string[] = []; + await diagnoseGitHubAuthFailure(env.gh, probe.status, (m) => diagnosis.push(m)); + throw new SkipError( + `${tool}: EXECUTOR_E2E_GITHUB_TOKEN cannot write issues on '${env.repo}' (HTTP ${probe.status}). ${diagnosis.join(" ")}`, + ); + } +} + +/** Deterministic, greppable issue title for a scenario. */ +function issueTitle(ctx: ScenarioContext, id: string): string { + return `${ctx.prefix(id)} scratch issue`; +} + +/** The env every scenario passes to `ado-aw execute`. */ +function executeEnv(env: GithubIssueEnv): Record { + // Stage 3 reads the credential from ADO_AW_GITHUB_TOKEN only + // (`ExecutionContext::github_token`); the harness's own + // EXECUTOR_E2E_GITHUB_TOKEN is not consulted by the binary. + return { ADO_AW_GITHUB_TOKEN: env.token }; +} + +/** + * Close an issue, tolerating the case where it was never created. + * + * `cleanup()` must not depend solely on state written by `assert()`: when the + * executor filed an issue but the record came back non-`succeeded` (e.g. + * `failure_with_data` from a temporary-ID registration error) the runner never + * calls `assert()`, so the number is unknown. Falling back to an exact-title + * search on the `ado-aw-det-*` marker closes the issue anyway. + */ +async function closeByNumberOrTitle( + env: GithubIssueEnv, + issueNumber: number | undefined, + title: string, +): Promise { + const number = issueNumber ?? (await findOpenIssueByTitle(env.gh, title)); + if (number === undefined) return; + await closeIssue(env.gh, number); +} + +/** Pull one record out of a run by its kebab-case tool name. */ +export function recordForTool( + records: ExecutedRecord[], + tool: string, +): ExecutedRecord { + const name = tool.replaceAll("-", "_"); + const record = records.find((r) => r.name === name); + if (!record) throw new Error(`no executed record for '${tool}'`); + return record; +} + +// --------------------------------------------------------------------------- +// create-github-issue +// --------------------------------------------------------------------------- + +interface CreateState extends GithubIssueEnv { + title: string; + issueNumber?: number; +} + +export const createGithubIssue: Scenario = { + id: "create-github-issue", + tool: "create-github-issue", + config: (_ctx, state) => ({ + "target-repo": state.repo, + "title-prefix": TITLE_PREFIX, + labels: [STATIC_LABEL], + "allowed-labels": [ALLOWED_LABEL_PATTERN], + }), + setup: async (ctx) => { + const env = resolveGithubIssueEnv("create-github-issue"); + await requireIssueWrite(env, "create-github-issue"); + return { ...env, title: issueTitle(ctx, "create-github-issue") }; + }, + ndjson: async (ctx, state) => ({ + title: state.title, + body: detBody(ctx, "create-github-issue"), + labels: [ALLOWED_AGENT_LABEL], + }), + env: async (_ctx, state) => executeEnv(state), + assert: async (ctx, state, record) => { + // Cleanup-critical: remember the number before any fallible assertion. + state.issueNumber = numResult(record, "number"); + if (strResult(record, "target_repo") !== state.repo) { + throw new Error( + `executor filed into '${strResult(record, "target_repo")}', expected '${state.repo}'`, + ); + } + + const issue = await getIssue(state.gh, state.issueNumber); + if (!issue) throw new Error(`issue #${state.issueNumber} was not created`); + + const expectedTitle = `${TITLE_PREFIX}${state.title}`; + if (issue.title !== expectedTitle) { + throw new Error(`issue title is '${issue.title}', expected '${expectedTitle}'`); + } + if (!issue.body?.includes(detBody(ctx, "create-github-issue"))) { + throw new Error("issue body does not carry the agent-supplied text"); + } + if (!issue.body.includes(FOOTER_MARKER)) { + throw new Error(`issue body is missing the '${FOOTER_MARKER}' traceability footer`); + } + const labels = issue.labels.map((l) => l.toLowerCase()); + for (const expected of [STATIC_LABEL, ALLOWED_AGENT_LABEL]) { + if (!labels.includes(expected)) { + throw new Error(`issue labels ${JSON.stringify(issue.labels)} are missing '${expected}'`); + } + } + }, + cleanup: async (_ctx, state) => + closeByNumberOrTitle(state, state.issueNumber, `${TITLE_PREFIX}${state.title}`), +}; + +// --------------------------------------------------------------------------- +// create-github-issue — allowed-labels rejection +// --------------------------------------------------------------------------- + +/** + * `allowed-labels` is **default-deny**: an empty/absent list rejects every + * agent-supplied label, and `["*"]` is the explicit opt-out. (Note the + * asymmetry with `set-github-issue-type.allowed`, which is default-allow.) + * Here the allowlist is non-empty but does not match, so the executor must + * reject the proposal and file nothing. + */ +export const createGithubIssueLabelDenied: Scenario = { + id: "create-github-issue-label-denied", + tool: "create-github-issue", + config: (_ctx, state) => ({ + "target-repo": state.repo, + "title-prefix": TITLE_PREFIX, + "allowed-labels": [ALLOWED_LABEL_PATTERN], + }), + setup: async (ctx) => { + const env = resolveGithubIssueEnv("create-github-issue-label-denied"); + await requireIssueWrite(env, "create-github-issue-label-denied"); + return { ...env, title: issueTitle(ctx, "create-github-issue-label-denied") }; + }, + ndjson: async (ctx, state) => ({ + title: state.title, + body: detBody(ctx, "create-github-issue-label-denied"), + labels: [DENIED_AGENT_LABEL], + }), + env: async (_ctx, state) => executeEnv(state), + expectedFailure: { + // Deliberately matches ONLY the "allowlist is configured but does not + // match" message. The other rejection message — "no `allowed-labels` + // configured" — is what the executor emits when it never read the config at + // all, so accepting both would let this scenario pass even when + // `allowed-labels` was silently discarded. See the config-drop note in + // tests/executor-e2e/README.md. + error: /labels not in allowed-labels/i, + }, + // Never reached: the runner short-circuits on a matching expectedFailure. + assert: async () => { + throw new Error("create-github-issue should have rejected the disallowed label"); + }, + // Belt and braces: if the executor ever regressed and filed the issue anyway, + // the title-marker search finds and closes it. + cleanup: async (_ctx, state) => + closeByNumberOrTitle(state, undefined, `${TITLE_PREFIX}${state.title}`), +}; + +// --------------------------------------------------------------------------- +// set-github-issue-type +// --------------------------------------------------------------------------- + +interface SetTypeState extends GithubIssueEnv { + title: string; + issueNumber: number; + issueType: string; +} + +/** + * Discover a named org issue type, or `SkipError`. + * + * `E2E_GITHUB_ISSUE_TYPE` forces a specific name for environments where the + * token cannot read org metadata but the type is known to exist. + */ +async function requireNamedIssueType( + env: GithubIssueEnv, + tool: string, + processEnv: NodeJS.ProcessEnv = process.env, +): Promise { + const explicit = cleanVar(processEnv.E2E_GITHUB_ISSUE_TYPE); + if (explicit) return explicit; + const { owner } = splitRepo(env.repo); + const types = await listOrgIssueTypes(env.gh, owner); + if (types.length === 0) { + throw new SkipError( + `${tool}: no native issue types are defined for '${owner}'. Issue types are an ` + + `organisation-level construct with no user-account equivalent, so a user-owned ` + + `scratch repo can never expose one. Set E2E_GITHUB_ISSUE_TYPE to force a name.`, + ); + } + return types[0]!; +} + +/** + * Create the scratch issue this scenario will mutate. + * + * Called only after every `SkipError` check has passed, so `setup()` never + * leaves an orphan behind: a throw before this point creates nothing, and a + * throw after it is torn down inline (the runner does not run `cleanup()` when + * `setup()` fails). + */ +async function seedIssue( + ctx: ScenarioContext, + env: GithubIssueEnv, + id: string, +): Promise<{ title: string; issueNumber: number }> { + const title = issueTitle(ctx, id); + const url = await createGitHubIssue(env.gh, title, detBody(ctx, id), [STATIC_LABEL]); + const match = url.match(/\/(\d+)$/); + if (!match) { + // The issue exists but we could not parse its number from the URL — close + // it by title so setup's failure does not leak an open issue. + await closeByNumberOrTitle(env, undefined, title).catch(() => {}); + throw new Error(`could not parse an issue number out of '${url}'`); + } + return { title, issueNumber: Number(match[1]) }; +} + +export const setGithubIssueType: Scenario = { + id: "set-github-issue-type", + tool: "set-github-issue-type", + config: (_ctx, state) => ({ "target-repo": state.repo }), + setup: async (ctx) => { + const env = resolveGithubIssueEnv("set-github-issue-type"); + await requireIssueWrite(env, "set-github-issue-type"); + const issueType = await requireNamedIssueType(env, "set-github-issue-type"); + const seeded = await seedIssue(ctx, env, "set-github-issue-type"); + return { ...env, ...seeded, issueType }; + }, + ndjson: async (_ctx, state) => ({ + issue_number: state.issueNumber, + issue_type: state.issueType, + }), + env: async (_ctx, state) => executeEnv(state), + assert: async (_ctx, state, record) => { + if (numResult(record, "number") !== state.issueNumber) { + throw new Error( + `executor targeted issue #${numResult(record, "number")}, expected #${state.issueNumber}`, + ); + } + const issue = await getIssue(state.gh, state.issueNumber); + if (issue?.type?.toLowerCase() !== state.issueType.toLowerCase()) { + throw new Error( + `issue #${state.issueNumber} has type '${issue?.type ?? "(none)"}', expected '${state.issueType}'`, + ); + } + }, + cleanup: async (_ctx, state) => closeByNumberOrTitle(state, state.issueNumber, state.title), +}; + +// --------------------------------------------------------------------------- +// set-github-issue-type — documented clear operation +// --------------------------------------------------------------------------- + +/** + * Probe whether the repo accepts a type clear (`{"type": ""}`). + * + * Where no issue types exist the clear is a legitimate no-op, but GitHub is + * free to reject the field outright — so probe once and `SkipError` instead of + * reporting a product failure. + */ +async function requireClearSupport( + env: GithubIssueEnv, + issueNumber: number, + tool: string, +): Promise { + const res = await patchIssue(env.gh, issueNumber, { type: "" }); + if (!res.ok) { + throw new SkipError( + `${tool}: GitHub rejected a native issue-type clear on '${env.repo}' (HTTP ${res.status}); ` + + `this repository does not support the issue-type field`, + ); + } +} + +export const setGithubIssueTypeClear: Scenario = { + id: "set-github-issue-type-clear", + tool: "set-github-issue-type", + config: (_ctx, state) => ({ "target-repo": state.repo }), + setup: async (ctx) => { + const env = resolveGithubIssueEnv("set-github-issue-type-clear"); + await requireIssueWrite(env, "set-github-issue-type-clear"); + const seeded = await seedIssue(ctx, env, "set-github-issue-type-clear"); + // Everything below can throw, and the runner will NOT call cleanup() for a + // setup failure — so tear the seeded issue down inline before rethrowing. + try { + // Give the issue a type first where one exists, so the clear is + // observable rather than a no-op. + let issueType = ""; + try { + issueType = await requireNamedIssueType(env, "set-github-issue-type-clear"); + } catch (err) { + if (!(err instanceof SkipError)) throw err; + } + if (issueType) { + const applied = await patchIssue(env.gh, seeded.issueNumber, { type: issueType }); + if (!applied.ok) issueType = ""; + } + await requireClearSupport(env, seeded.issueNumber, "set-github-issue-type-clear"); + if (issueType) { + await patchIssue(env.gh, seeded.issueNumber, { type: issueType }); + } + return { ...env, ...seeded, issueType }; + } catch (err) { + await closeByNumberOrTitle(env, seeded.issueNumber, seeded.title).catch(() => {}); + throw err; + } + }, + ndjson: async (_ctx, state) => ({ issue_number: state.issueNumber, issue_type: "" }), + env: async (_ctx, state) => executeEnv(state), + assert: async (_ctx, state, record) => { + if (numResult(record, "number") !== state.issueNumber) { + throw new Error( + `executor targeted issue #${numResult(record, "number")}, expected #${state.issueNumber}`, + ); + } + if (strResult(record, "issue_type") !== "") { + throw new Error( + `executor reported issue_type '${strResult(record, "issue_type")}', expected the clear sentinel ''`, + ); + } + const issue = await getIssue(state.gh, state.issueNumber); + if (issue?.type) { + throw new Error(`issue #${state.issueNumber} still has type '${issue.type}' after the clear`); + } + }, + cleanup: async (_ctx, state) => closeByNumberOrTitle(state, state.issueNumber, state.title), +}; + +// --------------------------------------------------------------------------- +// same-run temporary_id handoff +// --------------------------------------------------------------------------- + +interface HandoffState extends GithubIssueEnv { + title: string; + /** Named type when one is discoverable; "" exercises the clear path. */ + issueType: string; + /** Populated by assert() from the prior create record. */ + issueNumber?: number; +} + +/** + * The highest-value scenario: `create-github-issue` mints an issue under + * `temporary_id`, and `set-github-issue-type` — in the **same** + * `ado-aw execute` process — resolves that id to the real issue number. + * + * The registry backing this handoff (`ExecutionContext.resolved_github_issues`) + * is an in-process `Arc>>`, so a wrong REST shape or a scoping + * failure would be invisible to any test that runs the two tools separately. + * The proof is that the *type* record reports the number the *create* record + * actually produced. + */ +export const createGithubIssueTemporaryIdHandoff: Scenario = { + id: "create-github-issue-temporary-id-handoff", + // Primary tool is the CONSUMER of the temporary id; create-github-issue is + // staged ahead of it as a prior entry. + tool: "set-github-issue-type", + config: (_ctx, state) => ({ "target-repo": state.repo }), + setup: async (ctx) => { + const id = "create-github-issue-temporary-id-handoff"; + const env = resolveGithubIssueEnv(id); + await requireIssueWrite(env, id); + // Prefer a named type; fall back to the documented clear operation so the + // handoff still runs on a repo with no issue types. + let issueType = ""; + try { + issueType = await requireNamedIssueType(env, id); + } catch (err) { + if (!(err instanceof SkipError)) throw err; + } + return { ...env, title: issueTitle(ctx, id), issueType }; + }, + priorEntries: async (ctx, state): Promise => [ + { + tool: "create-github-issue", + config: { + "target-repo": state.repo, + labels: [STATIC_LABEL], + "require-temporary-id": true, + }, + entry: { + title: state.title, + body: detBody(ctx, "create-github-issue-temporary-id-handoff"), + temporary_id: TEMPORARY_ID, + }, + }, + ], + ndjson: async (_ctx, state) => ({ + issue_number: TEMPORARY_ID, + issue_type: state.issueType, + }), + env: async (_ctx, state) => executeEnv(state), + assert: async (_ctx, state, record, records) => { + // Cleanup-critical: capture the real number from the create record before + // any fallible assertion, so a later throw still tears the issue down. + const created = recordForTool(records, "create-github-issue"); + const createdNumber = numResult(created, "number"); + state.issueNumber = createdNumber; + + if (strResult(created, "temporary_id") !== TEMPORARY_ID) { + throw new Error( + `create-github-issue reported temporary_id '${strResult(created, "temporary_id")}', expected '${TEMPORARY_ID}'`, + ); + } + + // THE handoff assertion: the consumer must have resolved the temporary id + // to the issue the producer actually filed, in the same repository. + const resolvedNumber = numResult(record, "number"); + if (resolvedNumber !== createdNumber) { + throw new Error( + `temporary_id '${TEMPORARY_ID}' resolved to issue #${resolvedNumber}, but ` + + `create-github-issue filed #${createdNumber}`, + ); + } + const resolvedRepo = strResult(record, "target_repo"); + if (resolvedRepo !== strResult(created, "target_repo")) { + throw new Error( + `temporary_id '${TEMPORARY_ID}' resolved to repository '${resolvedRepo}', but ` + + `create-github-issue filed into '${strResult(created, "target_repo")}'`, + ); + } + + // Corroborate against GitHub itself so a fabricated result payload cannot + // satisfy the assertion above. + const issue = await getIssue(state.gh, createdNumber); + if (!issue) throw new Error(`issue #${createdNumber} does not exist on '${state.repo}'`); + if (issue.title !== state.title) { + throw new Error( + `issue #${createdNumber} is titled '${issue.title}', expected '${state.title}'`, + ); + } + if (state.issueType && issue.type?.toLowerCase() !== state.issueType.toLowerCase()) { + throw new Error( + `issue #${createdNumber} has type '${issue.type ?? "(none)"}', expected '${state.issueType}'`, + ); + } + }, + cleanup: async (_ctx, state) => + new Teardown() + .add("close handoff issue", () => closeByNumberOrTitle(state, state.issueNumber, state.title)) + .run(), +}; + +export const githubIssueScenarios: Scenario[] = [ + createGithubIssue, + createGithubIssueLabelDenied, + setGithubIssueType, + setGithubIssueTypeClear, + createGithubIssueTemporaryIdHandoff, +]; diff --git a/scripts/ado-script/src/executor-e2e/scenarios/index.ts b/scripts/ado-script/src/executor-e2e/scenarios/index.ts index a95651e9..9165e8fa 100644 --- a/scripts/ado-script/src/executor-e2e/scenarios/index.ts +++ b/scripts/ado-script/src/executor-e2e/scenarios/index.ts @@ -6,6 +6,7 @@ import type { Scenario } from "../scenario.js"; import { buildScenarios } from "./build.js"; import { createPullRequestScenarios } from "./create-pull-request.js"; import { gitScenarios } from "./git.js"; +import { githubIssueScenarios } from "./github-issue.js"; import { prScenarios } from "./pr.js"; import { signalScenarios } from "./signals.js"; import { wikiScenarios } from "./wiki.js"; @@ -20,4 +21,5 @@ export const allScenarios: Scenario[] = [ ...gitScenarios, ...buildScenarios, ...createPullRequestScenarios, + ...githubIssueScenarios, ]; diff --git a/src/safe_outputs/result.rs b/src/safe_outputs/result.rs index 317efc38..e96d1f36 100644 --- a/src/safe_outputs/result.rs +++ b/src/safe_outputs/result.rs @@ -221,16 +221,38 @@ impl ExecutionContext { .get(tool_name) .cloned() .map(|mut value| { - // Approval is compiler orchestration metadata, not executor - // configuration. Strip it before typed configs with - // deny_unknown_fields are deserialized. + // Compiler orchestration metadata, not executor configuration. + // Both keys are injected into EVERY tool config by Stage 3 + // (`main.rs` for `--source`, `compile/custom_tools.rs` for the + // `--resolved-config` production path), so a config struct + // declared `deny_unknown_fields` fails to deserialize unless + // they are stripped first. Because the error is swallowed + // below, that manifests as the operator's config being + // silently replaced by `Default::default()` rather than as a + // visible failure — keep this list in sync with every key the + // compiler injects. if let Some(object) = value.as_object_mut() { object.remove("require-approval"); + object.remove("staged"); } value }); let mut config: T = value - .and_then(|v| serde_json::from_value(v).ok()) + .map(|v| match serde_json::from_value(v) { + Ok(config) => config, + Err(error) => { + // Never fail silently: a config-shape mismatch here wipes + // every operator-supplied setting for the tool (target + // repos, allowlists, budgets), which is easy to mistake for + // a product bug at runtime. + log::warn!( + "Failed to deserialize config for tool '{tool_name}': {error}. \ + Falling back to defaults; operator-supplied settings for this \ + tool will NOT be applied." + ); + T::default() + } + }) .unwrap_or_default(); config.sanitize_config_fields(); config @@ -1189,4 +1211,72 @@ mod tests { assert!(ctx.pull_request_source_branch.is_none()); assert!(ctx.pull_request_target_branch.is_none()); } + + /// Build a context whose tool config carries the orchestration keys the + /// compiler injects into EVERY tool config. + fn ctx_with_injected_keys(tool: &str, mut config: serde_json::Value) -> ExecutionContext { + let object = config.as_object_mut().expect("config must be an object"); + // Mirrors `main.rs` (--source) and `compile/custom_tools.rs` + // (--resolved-config, the production path). + object.insert("staged".to_string(), serde_json::Value::Bool(false)); + object.insert( + "require-approval".to_string(), + serde_json::Value::Bool(false), + ); + let mut tool_configs = HashMap::new(); + tool_configs.insert(tool.to_string(), config); + ExecutionContext { + tool_configs, + ..Default::default() + } + } + + /// Regression guard for a silent config wipe. + /// + /// `CreateGithubIssueConfig` and `SetGithubIssueTypeConfig` are declared + /// `#[serde(deny_unknown_fields)]`, so the compiler-injected `staged` / + /// `require-approval` keys made deserialization fail. `get_tool_config` + /// swallowed the error and returned `Default::default()`, silently + /// discarding every operator setting (`target-repo`, `allowed-labels`, + /// budgets, …) instead of failing visibly. + #[test] + fn test_get_tool_config_survives_compiler_injected_orchestration_keys() { + let ctx = ctx_with_injected_keys( + "create-github-issue", + serde_json::json!({ + "target-repo": "octo/scratch", + "title-prefix": "[prefix] ", + "labels": ["static-label"], + "allowed-labels": ["agent-*"], + "require-temporary-id": true, + "max": 3, + }), + ); + let config: crate::safe_outputs::CreateGithubIssueConfig = + ctx.get_tool_config("create-github-issue"); + assert_eq!( + config.target_repo.as_deref(), + Some("octo/scratch"), + "operator target-repo must survive the injected orchestration keys" + ); + assert_eq!(config.title_prefix.as_deref(), Some("[prefix] ")); + assert_eq!(config.labels, vec!["static-label".to_string()]); + assert_eq!(config.allowed_labels, vec!["agent-*".to_string()]); + assert!(config.require_temporary_id); + assert_eq!(config.max, Some(3)); + } + + #[test] + fn test_get_tool_config_survives_injected_keys_for_set_github_issue_type() { + let ctx = ctx_with_injected_keys( + "set-github-issue-type", + serde_json::json!({ "target-repo": "octo/scratch", "allowed": ["Bug"] }), + ); + let config: crate::safe_outputs::SetGithubIssueTypeConfig = + ctx.get_tool_config("set-github-issue-type"); + assert_eq!(config.target_repo.as_deref(), Some("octo/scratch")); + // An empty `allowed` list is default-ALLOW, so a silent wipe here fails + // open — any issue type would be accepted. + assert_eq!(config.allowed, vec!["Bug".to_string()]); + } } diff --git a/tests/executor-e2e/README.md b/tests/executor-e2e/README.md index 116d62bf..4b92ab83 100644 --- a/tests/executor-e2e/README.md +++ b/tests/executor-e2e/README.md @@ -63,14 +63,121 @@ All deterministically-assertable ADO-write safe outputs plus the flagship scenario supplies only `ADO_AW_SELF_REPOSITORY_NAME` — matching what the compiler emits — so it also proves the executor resolves a repository from its name alone. +- **GitHub issues:** `create-github-issue`, `set-github-issue-type`, and the + same-run `temporary_id` handoff between them. These are the only scenarios + that assert against **GitHub** rather than ADO — see + [GitHub issue scenarios](#github-issue-scenarios) below. -Excluded (out of scope or GitHub-only): the GitHub-only `create-github-issue`. +Excluded (out of scope): none of the currently shipped safe outputs. > **Coverage note.** The signal scenarios (`noop`, `missing-tool`, > `missing-data`, `report-incomplete`) were previously exercised only by > now-deleted per-tool agentic smoke pipelines. Adding them here closes > the coverage gap while keeping the test deterministic. +## GitHub issue scenarios + +`create-github-issue` and `set-github-issue-type` had **zero runtime +coverage** before these scenarios: their only proof was a wiremock unit test, +and the last thing exercising `create-github-issue` end to end +(`smoke-failure-reporter`) was removed by the smoke-suite rework. + +| Scenario id | Tool | What it proves | +| --- | --- | --- | +| `create-github-issue` | `create-github-issue` | final title is `title-prefix` + the agent title; the body carries the agent text and the `` traceability footer; config-injected static labels merge with allowed agent labels | +| `create-github-issue-label-denied` | `create-github-issue` | `allowed-labels` is **default-deny** — an agent label outside the allowlist is rejected and no issue is filed | +| `set-github-issue-type` | `set-github-issue-type` | a native issue type is applied to an existing issue | +| `set-github-issue-type-clear` | `set-github-issue-type` | the documented `issue_type: ""` clear operation | +| `create-github-issue-temporary-id-handoff` | `set-github-issue-type` (with `create-github-issue` staged ahead of it) | the same-run `temporary_id` handoff | + +### Why the handoff scenario is shaped differently + +`create-github-issue` may mint a `temporary_id`, and +`set-github-issue-type.issue_number` accepts either a real number or that id. +The registry backing this (`ExecutionContext::resolved_github_issues`) is an +in-process `Arc>>` that is never persisted, so the handoff is +only observable inside a **single `ado-aw execute` invocation**. + +That matches production — a SafeOutputs job runs one `ado-aw execute` over the +whole `safe_outputs.ndjson` — but every other scenario here runs one entry per +invocation. The handoff scenario therefore uses the harness's `priorEntries` +hook (see `Scenario.priorEntries` in `scripts/ado-script/src/executor-e2e/scenario.ts`) +to stage `create-github-issue` as an extra NDJSON line ahead of its own, in the +same executor process. The assertion is that the `set-github-issue-type` record +reports the issue number `create-github-issue` actually filed. + +> **Why this can't be split across jobs.** Because the registry is per-process, +> putting `require-approval` on only one of the two tools would split Stage 3 +> into two `ado-aw execute` processes (`SafeOutputs` and `SafeOutputs_Reviewed`), +> and a `temporary_id` minted in one could not resolve in the other. The +> compiler rejects that configuration up front — +> `validate_github_issue_outputs_config` in `src/compile/common.rs` requires both +> tools to have the same *effective* `require-approval` setting, so the +> section-level default and a per-tool override are both accounted for. + +### A product bug these scenarios caught + +Adding this coverage immediately found a real defect in Stage 3 config +handling, now fixed in `ExecutionContext::get_tool_config` +(`src/safe_outputs/result.rs`). It is recorded here because it is exactly the +class of bug the wiremock unit tests structurally could not see. + +Stage 3 injects synthetic `staged` and `require-approval` keys into **every** +tool config (`src/main.rs` for `--source`; `src/compile/custom_tools.rs` for the +compiler-generated `--resolved-config` that production uses). +`CreateGithubIssueConfig` and `SetGithubIssueTypeConfig` are the only +safe-output configs declared `#[serde(deny_unknown_fields)]`, and neither +declares a `staged` field — so deserialization failed, `get_tool_config` +swallowed the error via `.ok().unwrap_or_default()`, and **the operator config +was silently replaced with `Default::default()`**. + +Observable effects: `target-repo` ignored (Stage 3 failed outright on +non-GitHub-backed ADO builds), `title-prefix` never applied, static +`labels`/`assignees` dropped, `allowed-labels` emptied so default-deny rejected +*every* agent label, `require-temporary-id` unenforced, and +`set-github-issue-type.allowed` never gating anything — the last of which +failed **open**. + +The unit tests missed it because they build an `ExecutionContext` directly with +a config map that has no `staged` key, i.e. a shape that never occurs in +production. The fix strips both orchestration keys and logs a warning instead of +silently defaulting; `result.rs` carries regression tests asserting an operator +config survives the injected keys. + +Note that `create-github-issue-label-denied` deliberately matches only the +`labels not in allowed-labels` message. The alternative message +(`no allowed-labels configured`) is precisely what the executor emitted when the +config was dropped, so accepting both would have let the scenario pass either +way — the failure mode this suite exists to prevent. + +### Close, don't delete + +GitHub has **no REST endpoint to delete an issue**. These scenarios are the only +ones in the suite that cannot tear down completely: `cleanup()` closes each +issue as `not_planned` instead. + +Every scratch issue title embeds the standard `ado-aw-det-$(Build.BuildId)-` +marker, so anything a cleanup misses is findable with a single search on the +scratch repository. Cleanup also does **not** depend solely on state captured in +`assert()` — when the executor filed an issue but the record came back +non-`succeeded`, `assert()` never runs, so cleanup falls back to an exact-title +search on that marker. + +Because issues accumulate (closed, never deleted), point these scenarios at a +scratch repository, not a canonical one. + +### Environment + +| Variable | Meaning | +| --- | --- | +| `EXECUTOR_E2E_GITHUB_TOKEN` | Reused from failure-issue filing. It must now also carry **Issues: write** on the scratch repository, because these scenarios create, mutate, and close issues. | +| `EXECUTOR_E2E_SCENARIO_ISSUE_REPO` | Optional. `owner/repo` for scratch issues; falls back to `EXECUTOR_E2E_ISSUE_REPO`. Set it to keep scenario issues away from the failure-report repository. | +| `E2E_GITHUB_ISSUE_TYPE` | Optional. Forces a native issue-type name for environments where the token cannot read org metadata but the type is known to exist. | + +There is deliberately **no default repo** for these scenarios: when neither +variable is set they skip rather than filing scratch issues onto +`githubnext/ado-aw`. + ### Scenarios that skip when a precondition is missing Some scenarios need optional infrastructure and **skip** (rather than fail) @@ -82,6 +189,17 @@ when it is not available: no wiki exists, both skip. - `add-build-tag`, `upload-build-attachment`, `upload-pipeline-artifact` — need a real current build (`BUILD_BUILDID`); they skip when run outside a pipeline. +- **All five GitHub issue scenarios** — need `EXECUTOR_E2E_GITHUB_TOKEN` and a + scratch repo (`EXECUTOR_E2E_SCENARIO_ISSUE_REPO` or `EXECUTOR_E2E_ISSUE_REPO`). + They also skip when the token authenticates but cannot write issues on that + repo, with the harness's auth diagnosis attached to the skip reason. +- `set-github-issue-type` and, on the named-type path, the handoff — need a + native issue type to exist. Issue types are an **organisation-level** + construct (`GET /orgs/{org}/issue-types`) with no user-account equivalent, so + a **user-owned scratch repo can never expose one** and these skip + permanently there. The handoff scenario stays runnable by falling back to the + documented `issue_type: ""` clear operation; `set-github-issue-type-clear` + skips only if GitHub rejects the clear outright. ## Naming / cleanup convention @@ -106,7 +224,11 @@ export EXECUTOR_E2E_ADO_REPO="agent-definitions" # Optional: # export EXECUTOR_E2E_GITHUB_TOKEN="" # export EXECUTOR_E2E_ISSUE_REPO="jamesadevine/ado-aw-issues" -# export E2E_QUEUE_PIPELINE_ID="" +# Optional: keep GitHub issue scenario scratch issues out of the failure-report repo +# export EXECUTOR_E2E_SCENARIO_ISSUE_REPO="/" +# Optional: force a native issue-type name (org-owned repos only) +# export E2E_GITHUB_ISSUE_TYPE="Bug" +# export E2E_QUEUE_PIPELINE_ID="" # Optional timeout tuning (milliseconds) for slow environments: # export EXECUTOR_E2E_REST_TIMEOUT_MS=30000 # per ADO REST call (default 30000) # export EXECUTOR_E2E_EXECUTE_TIMEOUT_MS=600000 # per `ado-aw execute` run (default 600000) @@ -147,9 +269,23 @@ In `https://dev.azure.com/msazuresphere/AgentPlayground`: --value ``` Do **not** place this token in a shared variable group. + + > The GitHub issue **scenarios** reuse this same token, so it needs + > **Issues: write** on the scratch repository — not just enough to file a + > failure report. When it can authenticate but not write, those scenarios + > skip with a diagnosis rather than failing the build. 4. Set `EXECUTOR_E2E_ISSUE_REPO=jamesadevine/ado-aw-issues`. Confirm the target repository has `executor-e2e-failure` and `pipeline-failure` labels. + *(Optional)* Set `EXECUTOR_E2E_SCENARIO_ISSUE_REPO` to a separate scratch + repository so the GitHub issue scenarios do not accumulate closed issues + alongside the failure reports. + + > `set-github-issue-type` and the named-type half of the handoff need a + > native issue type, which is an **organisation-level** construct. On a + > user-owned repo such as `jamesadevine/ado-aw-issues` they will skip on + > every run; point `EXECUTOR_E2E_SCENARIO_ISSUE_REPO` at an org-owned repo + > with issue types defined to enable them. 5. Set `E2E_QUEUE_PIPELINE_ID` to the `queue-target` definition ID (register [`queue-target.yml`](queue-target.yml) if it does not exist yet). It is a permanent, trigger-free, non-agentic pipeline that exists only to be diff --git a/tests/executor-e2e/azure-pipelines.yml b/tests/executor-e2e/azure-pipelines.yml index 207f7869..1fb1d62a 100644 --- a/tests/executor-e2e/azure-pipelines.yml +++ b/tests/executor-e2e/azure-pipelines.yml @@ -41,6 +41,11 @@ variables: EFFECTIVE_EXECUTOR_E2E_ADO_REPO: $[ coalesce(variables['EXECUTOR_E2E_ADO_REPO'], 'agent-definitions') ] EFFECTIVE_E2E_QUEUE_PIPELINE_ID: $[ coalesce(variables['E2E_QUEUE_PIPELINE_ID'], '') ] EFFECTIVE_E2E_WIKI_NAME: $[ coalesce(variables['E2E_WIKI_NAME'], '') ] + # GitHub issue scenarios: optional scratch-repo override and forced issue-type + # name. Both empty by default so the scenarios skip gracefully rather than + # filing scratch issues into an unintended repository. + EFFECTIVE_EXECUTOR_E2E_SCENARIO_ISSUE_REPO: $[ coalesce(variables['EXECUTOR_E2E_SCENARIO_ISSUE_REPO'], '') ] + EFFECTIVE_E2E_GITHUB_ISSUE_TYPE: $[ coalesce(variables['E2E_GITHUB_ISSUE_TYPE'], '') ] # Internal sparse crates.io mirror (Azure Artifacts). Declared in the cargo # config below so CargoAuthenticate@0 can attach credentials, and reused as # RustInstaller's crates.io override so the two never drift. @@ -138,3 +143,9 @@ steps: # Optional-precondition scenarios (skipped when unset): E2E_QUEUE_PIPELINE_ID: $(EFFECTIVE_E2E_QUEUE_PIPELINE_ID) E2E_WIKI_NAME: $(EFFECTIVE_E2E_WIKI_NAME) + # GitHub issue scenarios. They reuse EXECUTOR_E2E_GITHUB_TOKEN above, + # which must carry Issues:write on the scratch repo. When + # EXECUTOR_E2E_SCENARIO_ISSUE_REPO is unset they fall back to + # EXECUTOR_E2E_ISSUE_REPO, and skip when neither is set. + EXECUTOR_E2E_SCENARIO_ISSUE_REPO: $(EFFECTIVE_EXECUTOR_E2E_SCENARIO_ISSUE_REPO) + E2E_GITHUB_ISSUE_TYPE: $(EFFECTIVE_E2E_GITHUB_ISSUE_TYPE)