diff --git a/test-companion/src/answer.mjs b/test-companion/src/answer.mjs index 95f4f53..e94898f 100644 --- a/test-companion/src/answer.mjs +++ b/test-companion/src/answer.mjs @@ -5,16 +5,28 @@ import { StringDecoder } from "node:string_decoder" // Cap on an unterminated line; the agent's commands can print megabytes without a newline. export const MAX_PARTIAL_LINE = 1 << 20 -export function createAnswerCapture() { +/** + * `plainErrors`: also read `Error: …` lines. Only for the CLI's stderr; on stdout the + * agent's test output prints its own `Error:` lines (assertions, stack traces). + */ +export function createAnswerCapture({ plainErrors = true } = {}) { // Chunks can split a multi-byte character. const decoder = new StringDecoder("utf8") let pending = "" // Discarding the rest of a line that overran MAX_PARTIAL_LINE. let skipping = false let answer + let plainError + let structuredError const read = (line) => { const t = line.trim() + // The CLI prints why it stopped as `Error: …` on stderr. + const plain = plainErrors && /^Error:\s*(.+)$/.exec(t) + if (plain) { + plainError = plain[1] + return + } if (!t.startsWith("{") || !t.endsWith("}")) return let event try { @@ -22,7 +34,12 @@ export function createAnswerCapture() { } catch { return } - if (!event || typeof event !== "object" || event.event !== "answer") return + if (!event || typeof event !== "object") return + if (event.event === "error" && typeof event.message === "string" && event.message.trim()) { + structuredError = event.message.trim() + return + } + if (event.event !== "answer") return const text = typeof event.text === "string" ? event.text.trim() : "" if (text) answer = text } @@ -51,6 +68,9 @@ export function createAnswerCapture() { pending = "" }, answer: () => answer, + structuredError: () => structuredError, + plainError: () => plainError, + error: () => structuredError ?? plainError, pending: () => pending.length, } } @@ -58,18 +78,62 @@ export function createAnswerCapture() { /** Run a command, echoing stdout live while scanning it for the answer. Never rejects. */ export function runCapturingAnswer(command, args, childEnv) { return new Promise((resolve) => { - const capture = createAnswerCapture() - const child = spawn(command, args, { stdio: ["inherit", "pipe", "inherit"], env: childEnv }) + const capture = createAnswerCapture({ plainErrors: false }) + const errors = createAnswerCapture() + const child = spawn(command, args, { stdio: ["inherit", "pipe", "pipe"], env: childEnv }) child.stdout.on("data", (chunk) => { process.stdout.write(chunk) capture.push(chunk) }) + child.stderr.on("data", (chunk) => { + process.stderr.write(chunk) + errors.push(chunk) + }) child.stdout.on("error", () => {}) - child.on("error", () => resolve({ status: null, answer: undefined })) + child.stderr.on("error", () => {}) + child.on("error", () => resolve({ status: null, answer: undefined, error: undefined })) // `close`, not `exit`: stdout may still be draining at exit. child.on("close", (status) => { capture.end() - resolve({ status, answer: capture.answer() }) + errors.end() + // The CLI's own structured error is the precise reason; a plain line is the fallback. + // The CLI doesn't pass its child processes' stderr through, so the last plain + // `Error:` line on stderr is cobra's, i.e. the CLI's own. + const error = capture.structuredError() ?? errors.structuredError() ?? errors.plainError() + resolve({ status, answer: capture.answer(), error }) }) }) } + +/** + * Hide credentials before text reaches a pull request comment. GitHub masks secrets in + * job logs, not in comments posted through the API. + */ +export function redactSecrets(text, secrets = []) { + if (text === undefined || text === null) return text + let out = String(text) + // Longest first, so a secret containing another is replaced whole. + const known = [...new Set(secrets.map((s) => String(s ?? "").trim()).filter((s) => s.length >= 6))].sort( + (a, b) => b.length - a.length, + ) + for (const s of known) out = out.split(s).join("***") + // user:password@ in any URL. + return out.replace(/(\/\/)[^/\s:@]+:[^/\s@]+@/g, "$1***@") +} + +/** redactSecrets over every string in a JSON payload about to be sent to GitHub. */ +export function redactDeep(value, secrets = []) { + if (typeof value === "string") return redactSecrets(value, secrets) + if (Array.isArray(value)) return value.map((v) => redactDeep(v, secrets)) + if (value && typeof value === "object") { + return Object.fromEntries(Object.entries(value).map(([k, v]) => [k, redactDeep(v, secrets)])) + } + return value +} + +/** Values of environment variables whose names mark them as credentials. */ +export function secretsFrom(env = {}) { + return Object.entries(env) + .filter(([k]) => /(TOKEN|ACCESS_KEY|PRIVATE_KEY|SECRET|PASSWORD)/i.test(k)) + .map(([, v]) => v) +} diff --git a/test-companion/src/main.mjs b/test-companion/src/main.mjs index a93827c..3aedc62 100644 --- a/test-companion/src/main.mjs +++ b/test-companion/src/main.mjs @@ -5,7 +5,19 @@ import { join } from "node:path" import { tmpdir } from "node:os" import { planRun, contextArgs, withPullRequestHead, TC_EVENTS } from "./plan-run.mjs" import { readIntent, INTENT } from "./intent.mjs" -import { chooseRunsToCancel, runIdsFromChecks, latestFailedRun } from "./runs.mjs" +import { + chooseRunsToCancel, + runIdsFromChecks, + ciOnCommit, + parseRunReference, + matchFailedByName, + checkNamedRun, + renderCiStillRunning, + renderBuildNote, + classifyLookup, + prCommitShas, + LOOKUP_FAILED_REPLY, +} from "./runs.mjs" import { decideGate, GATE, describeChangeSize, ANSWER_JOBS } from "./gate.mjs" import { mayAsk, describeTestRepoAccess } from "./access.mjs" import { findTicket } from "./ticket.mjs" @@ -13,7 +25,7 @@ import { childEnvFor } from "./child-env.mjs" import { canIsolate, prepareSandbox, stopSandbox, handToAgent, writeAgentEnv, readAgentFile } from "./sandbox.mjs" import { agentEnvFrom, asAgentCommand, checkPrHandoff, readClaimResult } from "./sandbox-policy.mjs" import { startRelay } from "./relay.mjs" -import { runCapturingAnswer } from "./answer.mjs" +import { runCapturingAnswer, redactSecrets, redactDeep, secretsFrom } from "./answer.mjs" import { appCredentials, signJwt, @@ -21,11 +33,12 @@ import { appApi, installationLookupMessage, } from "./app-auth.mjs" -import { OUTCOMES, parseSummary, resolveOutcome, checkConclusion, stepShouldFail, approvalWasSpent } from "./outcome.mjs" +import { OUTCOMES, parseSummary, resolveOutcome, checkConclusion, stepShouldFail, approvalWasSpent, runDidNotWork } from "./outcome.mjs" import { renderJobSummary, renderCheckSummary, renderComment, + requestMarkerFor, chooseCommentAction, checkSha, checkSubjectFor, @@ -40,6 +53,8 @@ import { renderStopRemoved, proposalConsumedMarkerFor, proposalAlreadyUsed, + planSpentState, + unavailablePlanReply, proposalNeedsDropping, flattenPages, hasApproverEdits, @@ -56,6 +71,12 @@ import { shouldPushWork, changedPaths, splitPushable, + splitHandoffPaths, + savingFor, + isRequestedByPerson, + leaveOut, + leaveOutMessage, + listFiles, renderPushNote, needsCheckout, isSameRepo, @@ -69,9 +90,20 @@ import { ghEnvFrom, gitEnvFrom, GIT_SAFE_ARGS, + commitIdentity, + handedOffPrRequest, + classifyHandoffPush, + pushFailureReason, + existingHandoffPr, + nextHandoffStep, + draftRejected, + handoffReport, } from "./push-work.mjs" const env = process.env +// Taken before the App token replaces GH_TOKEN and the key is deleted, so both are still hidden. +const STARTUP_SECRETS = secretsFrom(env) +const secretsNow = () => [...STARTUP_SECRETS, ...secretsFrom(env)] const REPO = env.GITHUB_REPOSITORY ?? "" const RUN_URL = `${env.GITHUB_SERVER_URL ?? "https://github.com"}/${REPO}/actions/runs/${env.GITHUB_RUN_ID ?? ""}` // RUNNER_TEMP is cleared per job; /tmp may hold an earlier job's files on a reused runner. @@ -117,7 +149,8 @@ function runGh(args, { token, input } = {}) { } function ghRaw(args, body) { - return runGh(args, { token: env.GH_TOKEN, input: body ? JSON.stringify(body) : undefined }) + // Every comment, check and pull request we send passes through here, so it's hidden once. + return runGh(args, { token: env.GH_TOKEN, input: body ? JSON.stringify(redactDeep(body, secretsNow())) : undefined }) } /** `gh api`, returning parsed JSON or null. Never throws. */ @@ -135,6 +168,9 @@ function gh(args, body) { } } +// Who the Action's commits are by; the App's bot account once we act as it. +let identity = commitIdentity() + // The login our comments appear under: github-actions[bot], or [bot] with the App. // Used to tell our proposals from another bot's. let botLogin = "github-actions[bot]" @@ -157,6 +193,9 @@ const ALLOW_UNISOLATED = /^true$/i.test(String(env.TC_ACTION_ALLOW_UNISOLATED ?? /** The proposal this run is carrying out, so the report can link it. */ let approvedPlan +/** Which failed run a build-reading job looked at, and what it left out. */ +let buildNote = "" + // No --depth: it would make the full clone shallow. The URL carries its own credential // because the starter sets persist-credentials: false. function fetchAndCheckout(sha, branch) { @@ -225,7 +264,13 @@ async function appInstallationToken() { // Mask before first use. console.log(`::add-mask::${token}`) - if (appLogin) botLogin = appLogin + if (appLogin) { + botLogin = appLogin + // The bot account's id makes the noreply email GitHub links to the App. + const user = runGh(["api", `users/${encodeURIComponent(appLogin)}`, "--jq", ".id"], { token }) + if (user.status !== 0) warn(`Test Companion: could not look up ${appLogin}'s account, so commits use the plain Test Companion name.`) + identity = commitIdentity({ appSlug: found.data?.app_slug, appUserId: user.status === 0 ? String(user.stdout ?? "").trim() : "" }) + } return token } @@ -278,11 +323,12 @@ const GIT_BIN = trustedGitPath({ }, }) -function git(args, extraEnv = {}) { +function git(args, extraEnv = {}, input = undefined) { // Near-empty environment and no hooks: the agent has had write access to the tree. if (!GIT_BIN) return { ok: false, out: "", raw: "", err: "no trusted git binary" } const r = spawnSync(GIT_BIN, [...GIT_SAFE_ARGS, ...args], { encoding: "utf8", + input, env: { ...gitEnvFrom(env), ...extraEnv }, }) // `raw` keeps porcelain's leading space; trimming would lose the first line's status column. @@ -444,6 +490,8 @@ async function main() { const CARRIES_A_PLAN = new Set(["test-pr"]) // Jobs that cannot run without knowing which build failed. const NEEDS_A_BUILD = new Set(["rca", "fix", "verdict"]) + // How to ask each of those for a named run. + const ASK_WORD = { rca: "explain", fix: "fix", verdict: "find the bad PR" } const COMMENT_TRIGGERS = new Set(["issue_comment"]) if (COMMENT_TRIGGERS.has(env.GITHUB_EVENT_NAME) || plan.requiresWriteAccess) { const who = plan.commenter ?? readEvent().comment?.user?.login @@ -464,6 +512,8 @@ async function main() { let proposing = false let approvalNote = "" if (plan.event === TC_EVENTS.PR_COMMENT) { + // Kept for naming a run: an approval deletes plan.context.instructions below. + const requestText = plan.context.instructions // Fetched before reading intent: with a plan waiting, "go, and fix case 3" is an approval with an edit. const comments = flattenPages(gh(["api", `repos/${REPO}/issues/${plan.context.pr}/comments`, "--paginate", "--slurp"])) let waiting = findProposal(comments, plan.context.sha, Date.now(), botLogin) @@ -492,12 +542,8 @@ async function main() { if (intent.kind === INTENT.GO && intent.proposalId) { waiting = findProposal(comments, plan.context.sha, Date.now(), botLogin, { id: intent.proposalId }) if (!waiting) { - replyOnPr( - plan.context.pr, - `I have no plan \`#${intent.proposalId}\` still waiting on \`${String(plan.context.sha).slice(0, 7)}\`. ` + - `It may have been approved, dropped, or written against an earlier commit. ` + - `Reply **go** on its own to approve the most recent one.`, - ) + const spentAs = planSpentState(comments, intent.proposalId, botLogin) + replyOnPr(plan.context.pr, unavailablePlanReply(spentAs, intent.proposalId, plan.context.sha)) finishSkipped("proposal-not-found") } } @@ -587,7 +633,7 @@ async function main() { ) if (pending) rememberCheck(plan.context.sha, pending) // Per job, so approving one doesn't spend another job's plan. - consumedMarker = proposalConsumedMarkerFor(plan.context.sha, plan.workflow) + consumedMarker = proposalConsumedMarkerFor(plan.context.sha, plan.workflow, waiting?.id) approvedPlan = waiting?.url ? { id: waiting.id, url: waiting.url } : undefined // The approved plan, edits included, goes in a file: --context strips punctuation. @@ -620,22 +666,66 @@ async function main() { if (gate.preview || intent.preview) plan.preview = true // Last, because the job isn't final until approval is handled ("go" names none). - // rca, fix and verdict need a failed build: default to the newest red run on this commit. + // rca, fix and verdict need a failed build: one the comment names, else the newest red run on this commit. if (NEEDS_A_BUILD.has(plan.workflow) && !plan.context.build) { - const runs = gh(["api", `repos/${REPO}/actions/runs?head_sha=${plan.context.sha}&per_page=100`]) - const failed = latestFailedRun(runs?.workflow_runs, { - selfRunId: env.GITHUB_RUN_ID, - selfWorkflowName: env.TC_ACTION_SELF_WORKFLOW, - }) - if (!failed) { - replyOnPr( - plan.context.pr, - `Nothing has failed on \`${String(plan.context.sha).slice(0, 7)}\`, so there is no build for me to look at. Re-run me once something is red, or say what you want tested instead.`, + const short = String(plan.context.sha).slice(0, 7) + const named = parseRunReference(requestText, { repo: REPO, serverUrl: env.GITHUB_SERVER_URL }) + if (named?.error) { + replyOnPr(plan.context.pr, named.error) + finishSkipped("run-not-usable", "link") + } + if (named?.id) { + const lookup = classifyLookup(ghRaw(["api", `repos/${REPO}/actions/runs/${named.id}`])) + if (lookup.failed) { + replyOnPr(plan.context.pr, LOOKUP_FAILED_REPLY) + finishSkipped("lookup-failed", named.id) + } + const listed = prCommitShas( + gh(["api", `repos/${REPO}/pulls/${plan.context.pr}/commits?per_page=100`, "--paginate", "--slurp"]), + plan.context.sha, ) - finishSkipped("nothing-failed", plan.workflow) + const check = checkNamedRun(lookup.found, { + prShas: listed.shas, + shasComplete: listed.complete, + selfRunId: env.GITHUB_RUN_ID, + selfWorkflowName: env.TC_ACTION_SELF_WORKFLOW, + }) + if (!check.ok) { + replyOnPr(plan.context.pr, `I can't use run \`${named.id}\`: ${check.why}`) + finishSkipped("run-not-usable", named.id) + } + plan.context.build = check.url + buildNote = renderBuildNote(check, { ask: ASK_WORD[plan.workflow] }) + } else { + const runs = gh(["api", `repos/${REPO}/actions/runs?head_sha=${plan.context.sha}&per_page=100`]) + if (!Array.isArray(runs?.workflow_runs)) { + // A failed lookup is not "nothing failed". + replyOnPr(plan.context.pr, LOOKUP_FAILED_REPLY) + finishSkipped("lookup-failed", plan.workflow) + } + const ci = ciOnCommit(runs?.workflow_runs, { + selfRunId: env.GITHUB_RUN_ID, + selfWorkflowName: env.TC_ACTION_SELF_WORKFLOW, + }) + const chosen = matchFailedByName(requestText, ci.failed) ?? ci.failed[0] + if (!chosen) { + // A build still going is not "nothing failed": its failures just aren't in yet. + replyOnPr( + plan.context.pr, + ci.running.length + ? renderCiStillRunning(plan.context.sha, ci.running) + : `Nothing has failed on \`${short}\`, so there is no build for me to look at. Re-run me once something is red, or say what you want tested instead.`, + ) + finishSkipped(ci.running.length ? "ci-still-running" : "nothing-failed", plan.workflow) + } + plan.context.build = chosen.url + buildNote = renderBuildNote(chosen, { + otherFailed: ci.failed.filter((r) => r !== chosen), + running: ci.running, + ask: ASK_WORD[plan.workflow], + }) } - plan.context.build = failed - notice(`Test Companion: reading the failure from ${failed}`) + notice(`Test Companion: reading the failure from ${plan.context.build}`) } } @@ -663,11 +753,19 @@ async function main() { sameRepo: isSameRepo(prForTicket?.head?.repo?.full_name, REPO), branch: plan.context.branch, headAuthorEmail: git(["log", "-1", "--format=%ae"]).out, + ourEmails: [identity.email], // The earlier checkout is advisory, so confirm we are on the reviewed commit before pushing. onWantedCommit: !needsCheckout({ headSha: git(["rev-parse", "HEAD"]).out, wantSha: plan.context.sha }), defaultBranch: defaultBranchName(), + workflow: plan.workflow, + // Asked for by a person may build on our commit; automation keeps the loop guard. + requestedByPerson: isRequestedByPerson({ + isPrComment: plan.event === TC_EVENTS.PR_COMMENT, + eventName: env.GITHUB_EVENT_NAME, + sender: readEvent().sender, + }), }) - if (plan.context.branch && !plan.preview && !saving.ok) { + if (plan.context.branch && !plan.preview && !saving.ok && !saving.quiet) { notice(`Nothing will be saved to ${plan.context.branch}: ${saving.why}.`) } @@ -782,10 +880,13 @@ async function main() { }) // automate and fix hand back a pull request; other jobs save to this branch. - const workNote = handoffNote ?? saveWork(saving, plan) + // Re-checked with the job the pipeline chose: an event run didn't know it before the run. + // summary.workflow comes from a file the agent can write; savingFor can only make saving stricter. + const workNote = handoffNote ?? saveWork(savingFor(saving, plan.workflow ?? summary.workflow), plan) const view = { workflow: plan.workflow ?? summary.workflow, + planWorkflow: plan.workflow, approvedPlan, workNote, extraChecks, @@ -801,11 +902,18 @@ async function main() { askedAt: plan.askedAt, // rca and verdict leave no artefact: the answer is the agent's closing words. The result // file is the fallback, only for ANSWER_JOBS (for generate it holds test cases). - answer: run.answer || (hasResult && ANSWER_JOBS.has(plan.workflow ?? summary.workflow) ? readResult() : undefined), + answer: redactSecrets( + run.answer || (hasResult && ANSWER_JOBS.has(plan.workflow ?? summary.workflow) ? readResult() : undefined), + secretsNow(), + ), failedTests, + // Only where the run didn't work, so a stray error line never colours a success. + errorLine: runDidNotWork(outcome) ? redactSecrets(run.error, secretsNow()) : undefined, + buildNote, } - // Proposing: post what it would do and stop. Nothing has been written. - if (proposing) { + // Proposing: post what it would do and stop. A run that broke has nothing to approve, + // so it is reported like any other run, which also completes its check. + if (proposing && !runDidNotWork(outcome)) { const sha = checkSha(plan.context, summary, env.GITHUB_SHA) addToJobSummary(renderJobSummary(view)) const size = describeChangeSize(pull) @@ -892,7 +1000,7 @@ function saveWork(saving, plan) { if (unexpected.length) { return [ `**Could not save this work to \`${branch}\`:** the repository's git config carries`, - `${unexpected.map((k) => `\`${k}\``).join(", ")}, which \`actions/checkout\` does not write. Config is`, + `${listFiles(unexpected)}, which \`actions/checkout\` does not write. Config is`, 'where git keeps its "run this program" hooks and its URL rewrites, so Test Companion will not', "commit against a config it does not recognise. Nothing was committed or pushed — the run's", "changes are in the job's workspace only.", @@ -924,7 +1032,7 @@ function saveWork(saving, plan) { `Written by Test Companion for ${plan.context.pr ? `#${plan.context.pr}` : plan.context.sha}.`, `Run: ${RUN_URL}`, ].join("\n") - const committed = git(["commit", "-m", body]) + const committed = git(["-c", `user.name=${identity.name}`, "-c", `user.email=${identity.email}`, "commit", "-m", body]) if (!committed.ok) { return renderPushNote({ branch, @@ -956,7 +1064,7 @@ function isolateAgent(plan) { let why = can.why if (can.ok) { try { - const layout = prepareSandbox({ repoDir: process.cwd(), tcPath: TC_PATH }) + const layout = prepareSandbox({ repoDir: process.cwd(), tcPath: TC_PATH, identity }) notice("Test Companion: the agent runs as a separate, locked-down user with no access to this job's tokens.") return layout } catch (e) { @@ -1104,7 +1212,7 @@ function openHandedOffPr(path, plan) { if (!GIT_BIN) return "**Could not open the pull request:** no trusted `git` was found on this runner." const unexpected = unexpectedConfigKeys(git(CONFIG_READ_ARGS).raw) if (unexpected.length) { - return `**Did not open a pull request:** the repository's git config carries ${unexpected.map((k) => `\`${k}\``).join(", ")}, which \`actions/checkout\` does not write.` + return `**Did not open a pull request:** the repository's git config carries ${listFiles(unexpected)}, which \`actions/checkout\` does not write.` } const tip = git(["rev-parse", "--verify", "--quiet", `refs/heads/${req.branch}^{commit}`]) if (!tip.ok || !tip.out) return `**Did not open a pull request:** the run did not leave a \`${req.branch}\` branch.` @@ -1121,29 +1229,73 @@ function openHandedOffPr(path, plan) { paths.push(fields[i + 1]) if (meta[1] === "120000") links.add(fields[i + 1]) } - const { skip } = splitPushable(paths, { isSymlink: (p) => links.has(p) }) - if (skip.length) { - return `**Did not open a pull request:** the branch changes files Test Companion never pushes: ${skip.map((f) => `\`${f}\``).join(", ")}.` + const { refused, dropped } = splitHandoffPaths(paths, { isSymlink: (p) => links.has(p) }) + if (refused.length) { + return `**Did not open a pull request:** the branch changes files Test Companion never pushes: ${listFiles(refused)}.` + } + // Always one squashed commit on the base, under our name: the agent's own commits (and + // anything it added then deleted in them) never reach GitHub, and what was checked above is + // exactly what is pushed. + const indexFile = join(REPORT_DIR, `tc-handoff-index-${process.pid}`) + let left + try { + left = leaveOut({ + git, + base: from.out, + tip: tip.out, + paths: dropped, + identity, + message: redactSecrets(leaveOutMessage(req.title, dropped), secretsNow()), + indexFile, + }) + } finally { + rmSync(indexFile, { force: true }) + } + if (left.error) return `**Could not open the pull request:** ${left.error}.` + if (left.empty) { + return dropped.length + ? `**Did not open a pull request:** the run's only changes were files Test Companion never commits: ${listFiles(dropped)}.` + : `**Did not open a pull request:** the run's branch has no changes.` } + const head = left.sha + const leftOutNote = dropped.length ? ` Left out ${listFiles(dropped)}, which Test Companion never commits.` : "" const target = pushUrlFor(REPO, env.GITHUB_SERVER_URL) if (!target) return `**Could not open the pull request:** bad repository name "${REPO}".` - const pushed = git(["push", target, `${tip.out}:refs/heads/${req.branch}`], pushCredentialEnv(env.GH_TOKEN, target)) - if (!pushed.ok) { - return /rejected|non-fast-forward|already exists/i.test(pushed.err) - ? `**Already done:** \`${req.branch}\` already exists, so this job's pull request was opened by an earlier run.` - : `**Could not open the pull request:** ${pushed.err || "git push failed"}.` - } - // actions/checkout doesn't record the remote HEAD, so the CLI's base is only a fallback. - const opened = ghRaw(["api", `repos/${REPO}/pulls`, "--method", "POST", "--input", "-"], { - head: req.branch, - base: defaultBranchName() || req.base, - title: req.title, - body: req.body, + const onReviewedCommit = String(headBeforeRun ?? "").toLowerCase() === String(plan.context.sha ?? "").toLowerCase() + const request = handedOffPrRequest({ + req, + pr: plan.context.pr, + prBranch: plan.context.branch, + defaultBranch: defaultBranchName(), + onReviewedCommit, }) + const baseNote = + plan.context.pr && !onReviewedCommit + ? ` It targets \`${request.base}\`, not #${plan.context.pr}'s branch, because the run did not start on that pull request's commit.` + : "" + const pushed = git(["push", target, `${head}:refs/heads/${req.branch}`], pushCredentialEnv(env.GH_TOKEN, target)) + const pushKind = classifyHandoffPush(pushed) + let existing + if (pushKind === "already") { + // The branch is from an earlier run of this job on this commit; never reopen its closed PR. + const owner = REPO.split("/")[0] + existing = existingHandoffPr(gh(["api", `repos/${REPO}/pulls?head=${encodeURIComponent(`${owner}:${req.branch}`)}&state=all`])) + } + const step = nextHandoffStep(pushKind, existing) + if (step.action === "stop") { + const why = step.unknown ? "" : pushFailureReason(pushed.err) + return `${handoffReport(step, { branch: req.branch })} ${why ? `${why}.` : ""}`.trim() + } + if (step.action === "report") return handoffReport(step, { branch: req.branch }) + + const create = (body) => ghRaw(["api", `repos/${REPO}/pulls`, "--method", "POST", "--input", "-"], body) + let opened = create(request) + if (draftRejected(opened)) opened = create({ ...request, draft: false }) if (opened.status !== 0) { - return /already exists/i.test(`${opened.stdout}${opened.stderr}`) - ? `**Already done:** a pull request for \`${req.branch}\` is already open.` - : `**Pushed \`${req.branch}\` but could not open the pull request:** ${String(opened.stderr ?? "").trim().split("\n")[0]}.` + const why = String(opened.stderr ?? "").trim().split("\n")[0] + return step.earlier + ? `**Could not open a pull request** for an earlier run's ${listFiles([req.branch])}: ${why}.` + : `**Pushed ${listFiles([req.branch])} but could not open the pull request:** ${why}.` } let url = "" try { @@ -1151,9 +1303,14 @@ function openHandedOffPr(path, plan) { } catch { // The link is a nicety. } - return `**Opened a pull request** with this run's changes${url ? `: ${url}` : ` from \`${req.branch}\``}.` + const where = url ? `: ${url}` : ` from ${listFiles([req.branch])}` + // This run's notes describe this run's tree, not what an earlier run left on the branch. + return step.earlier + ? `**Opened a pull request** from an earlier run's branch for this commit${where}.` + : `**Opened a pull request** with this run's changes${where}.${leftOutNote}${baseNote}` } + /** Create or update this run's check on `sha`. */ function postCheck(outcome, view, sha, annotations = [], status = "completed", title = undefined) { if (!sha) return @@ -1325,7 +1482,7 @@ function sweepExpiredProposals() { postCheck(OUTCOMES.COULD_NOT_RUN, { outcome: OUTCOMES.COULD_NOT_RUN, summary: {}, runUrl: RUN_URL }, sha) replyOnPr( pr.number, - `That proposal went unanswered, so I have dropped it. The check now says Action required — comment again and I will put up a fresh one.\n\n${proposalDroppedMarkerFor(sha, stale?.workflow)}`, + `That proposal went unanswered, so I have dropped it. The check now says Action required — comment again and I will put up a fresh one.\n\n${proposalDroppedMarkerFor(sha, stale?.workflow, stale?.id)}`, ) notice(`Test Companion: dropped a stale proposal on #${pr.number}.`) } @@ -1379,12 +1536,18 @@ function markPlanSpent(planId, marker) { gh(["api", `repos/${REPO}/issues/comments/${planId}`, "--method", "PATCH", "--input", "-"], { body: `${body}\n\n${marker}` }) } +/** + * One reply per request: a run asked for in a comment gets its own report, keyed to that + * comment. Automatic runs, with no request to answer, keep one status comment per PR updated. + */ function postComment(pr, view) { + const requestId = env.GITHUB_EVENT_NAME === "issue_comment" ? readEvent().comment?.id : undefined // --slurp: without it gh prints one array per page and JSON.parse fails. const existing = flattenPages(gh(["api", `repos/${REPO}/issues/${pr}/comments`, "--paginate", "--slurp"])) - const choice = chooseCommentAction(existing, botLogin) + const choice = chooseCommentAction(existing, botLogin, { requestId }) // An approved proposal must stop looking like one. - const body = { body: renderComment(view) + (view.consumedMarker ? `\n${view.consumedMarker}` : "") } + const marker = requestMarkerFor(requestId) + const body = { body: renderComment(view, marker) + (view.consumedMarker ? `\n${view.consumedMarker}` : "") } if (choice.action === "update") { gh(["api", `repos/${REPO}/issues/comments/${choice.id}`, "--method", "PATCH", "--input", "-"], body) } else { diff --git a/test-companion/src/outcome.mjs b/test-companion/src/outcome.mjs index 7db4142..bacf282 100644 --- a/test-companion/src/outcome.mjs +++ b/test-companion/src/outcome.mjs @@ -101,6 +101,9 @@ export const BUILD_EFFECTS = Object.freeze({ const RUN_DID_NOT_WORK = new Set([OUTCOMES.RUN_BROKE, OUTCOMES.COULD_NOT_RUN, OUTCOMES.STOPPED_AT_LIMIT]) +/** The run broke, never started or hit a limit: there is nothing to approve. */ +export const runDidNotWork = (outcome) => RUN_DID_NOT_WORK.has(outcome) + /** * Whether the Action's step goes red. The check is unaffected. Unknown values * act as the default, not `report`; `off` is a legacy synonym for `report`. diff --git a/test-companion/src/plan-run.mjs b/test-companion/src/plan-run.mjs index cfa55d7..673d54d 100644 --- a/test-companion/src/plan-run.mjs +++ b/test-companion/src/plan-run.mjs @@ -142,6 +142,9 @@ function route({ eventName, payload, inputs, repo, sha, ref }) { function fromComment(payload, inputs) { if (payload.action !== "created") return skip("comment-edited", payload.action) if (!payload.issue?.pull_request) return skip("not-a-pull-request") + // Our own comments quote "@browserstack go"; reacting to them loops and can push a real request out of the queue. + const author = payload.comment?.user + if (author?.type === "Bot" || /\[bot\]$/i.test(author?.login ?? "")) return skip("bot-comment", author?.login) const body = payload.comment?.body ?? "" const addressed = MENTION.test(body) diff --git a/test-companion/src/push-work.mjs b/test-companion/src/push-work.mjs index 7539566..197eb89 100644 --- a/test-companion/src/push-work.mjs +++ b/test-companion/src/push-work.mjs @@ -6,18 +6,77 @@ /** Must match the identity action.yml configures. */ export const BOT_EMAIL = "test-companion[bot]@users.noreply.github.com" -export function authoredByUs(email) { - return ( - String(email ?? "") +export const BOT_NAME = "Test Companion" + +/** + * The identity commits are made under. As the GitHub App, its bot account, so commits + * show its name and logo; otherwise the fixed Test Companion identity. + */ +export function commitIdentity({ appSlug, appUserId } = {}) { + const slug = String(appSlug ?? "").trim() + const id = String(appUserId ?? "").trim() + if (!slug || !/^\d+$/.test(id)) return { name: BOT_NAME, email: BOT_EMAIL } + return { name: `${slug}[bot]`, email: `${id}+${slug}[bot]@users.noreply.github.com` } +} + +/** Slugs of the GitHub App Test Companion may act as. */ +export const APP_SLUGS = Object.freeze(["browserstack"]) + +/** + * Is this commit email ours? Includes the App's noreply address whatever identity this + * run has: a run that fell back to the workflow token must still see the App's commits. + */ +export function authoredByUs(email, ours = [], { appSlugs = APP_SLUGS } = {}) { + const norm = (v) => + String(v ?? "") .trim() - .toLowerCase() === BOT_EMAIL - ) + .toLowerCase() + const e = norm(email) + if (!e) return false + if (e === BOT_EMAIL || ours.map(norm).includes(e)) return true + const app = /^\d+\+([a-z0-9-]+)\[bot\]@users\.noreply\.github\.com$/.exec(e) + return Boolean(app && appSlugs.map(norm).includes(app[1])) +} + +/** Jobs whose result is a report or Test Management cases, never code on the branch. */ +export const NO_CODE_OUTPUT = new Set(["generate", "rca", "verdict", "validate"]) + +/** + * The save decision once the job is known. Event runs learn their job only from the + * pipeline file, after shouldPushWork ran without it. + */ +export function savingFor(saving, job) { + if (NO_CODE_OUTPUT.has(String(job ?? ""))) return { ok: false, quiet: true, why: `${job} does not change code` } + return saving +} + +/** + * Was this run asked for by a person? A pull request comment (bots are filtered out before + * this) or a manual run started by a human account. A manual run started by automation, such + * as a workflow dispatched with a token, doesn't count, so it keeps the own-commit loop guard. + */ +export function isRequestedByPerson({ isPrComment, eventName, sender } = {}) { + if (isPrComment) return true + if (eventName !== "workflow_dispatch") return false + return sender?.type === "User" && !/\[bot\]$/i.test(String(sender?.login ?? "")) } /** May this run push? Every check fails closed. */ -export function shouldPushWork({ preview, sameRepo, branch, headAuthorEmail, onWantedCommit, defaultBranch } = {}) { +export function shouldPushWork({ + preview, + sameRepo, + branch, + headAuthorEmail, + onWantedCommit, + defaultBranch, + workflow, + ourEmails = [], + requestedByPerson = false, +} = {}) { // A shell command the agent ran during preview is outside the brain's write refusal. if (preview) return { ok: false, why: "this is a preview run, which writes nothing" } + // Anything such a job leaves in the checkout is a by-product, not work to save. + if (NO_CODE_OUTPUT.has(String(workflow ?? ""))) return { ok: false, quiet: true, why: `${workflow} does not change code` } // Never push to a fork's branch with pull_request_target credentials. Unknown counts as no. if (sameRepo !== true) return { ok: false, why: "the change does not come from this repository" } if (!String(branch ?? "").trim()) return { ok: false, why: "there is no branch to push to" } @@ -30,8 +89,11 @@ export function shouldPushWork({ preview, sameRepo, branch, headAuthorEmail, onW if (base && String(branch).trim().toLowerCase() === base) { return { ok: false, why: "that is the repository's default branch" } } - // Loop guard: never push in reaction to our own commit. - if (authoredByUs(headAuthorEmail)) return { ok: false, why: "this run is reacting to our own commit" } + // Loop guard: an automatic run never pushes in reaction to our own commit. A run a person + // asked for or approved can't loop, so it may build on what an earlier run saved. + if (!requestedByPerson && authoredByUs(headAuthorEmail, ourEmails)) { + return { ok: false, why: "this run is reacting to our own commit" } + } return { ok: true } } @@ -90,6 +152,9 @@ const NEVER_COMMIT = [ /^\.testcompanion$/i, /^blob-report$/i, /\.log$/i, + // Written by BrowserStack tooling the agent runs; never part of the work. + /^browserstack\.err$/i, + /^\.browserstack/i, ] /** @@ -110,9 +175,41 @@ export function splitPushable(paths, { isSymlink = () => false } = {}) { return { push, skip } } +/** + * For a branch the agent handed back: files under `.github/` and symlinks are + * refused outright; other never-committed files (tool by-products, logs, .env) + * are dropped from the pull request instead of blocking it. + */ +export function splitHandoffPaths(paths, { isSymlink = () => false } = {}) { + const refused = [] + const dropped = [] + for (const path of splitPushable(paths, { isSymlink }).skip) { + const dangerous = isSymlink(path) || String(path).split("/").some((s) => s.toLowerCase() === ".github") + ;(dangerous ? refused : dropped).push(path) + } + return { refused, dropped } +} + +/** A file name as markdown code, safe whatever characters the agent put in it. */ +export function codeSpan(name) { + const text = String(name ?? "").replace(/[\r\n\t]/g, " ") + const longest = Math.max(0, ...(text.match(/`+/g) ?? []).map((run) => run.length)) + const fence = "`".repeat(longest + 1) + const pad = longest > 0 || text.startsWith("`") || text.endsWith("`") ? " " : "" + return `${fence}${pad}${text}${pad}${fence}` +} + +/** File names, at most `limit` of them, then how many more. */ +export function listFiles(names, { limit = 10, render = codeSpan, separator = ", " } = {}) { + const all = names ?? [] + const shown = all.slice(0, limit).map(render) + if (all.length > limit) shown.push(`and ${all.length - limit} more`) + return shown.join(separator) +} + /** The pull request note about the push, including when it failed. */ export function renderPushNote({ branch, sha, files = [], skipped = [], error } = {}) { - const list = files.map((f) => `- \`${f}\``).join("\n") + const list = listFiles(files, { render: (f) => `- ${codeSpan(f)}`, separator: "\n", limit: 50 }) if (error) { return [ `**Could not save this work to \`${branch}\`:** ${error}`, @@ -123,7 +220,7 @@ export function renderPushNote({ branch, sha, files = [], skipped = [], error } } const lines = [`**Saved to \`${branch}\`** as \`${String(sha ?? "").slice(0, 7)}\` — ${countOf(files)}:`, "", list] if (skipped.length) { - lines.push("", `Not committed, because Test Companion never commits these: ${skipped.map((f) => `\`${f}\``).join(", ")}.`) + lines.push("", `Not committed, because Test Companion never commits these: ${listFiles(skipped)}.`) } return lines.join("\n") } @@ -191,6 +288,8 @@ export function gitEnvFrom(env = {}) { out.GIT_ASKPASS = "" out.SSH_ASKPASS = "" out.GIT_ATTR_NOSYSTEM = "1" + // File names come from the agent: `:!x` must name a file, not mean "everything but x". + out.GIT_LITERAL_PATHSPECS = "1" return out } @@ -297,3 +396,130 @@ export const GIT_SAFE_ARGS = Object.freeze([ "-c", "core.attributesFile=/dev/null", ]) + +/** + * The pull request to open for work the CLI handed back. Asked on a pull request, + * it targets that pull request's branch, so it carries only the new change. + */ +export function handedOffPrRequest({ req, pr, prBranch, defaultBranch, onReviewedCommit = true } = {}) { + // The changes were measured from where the run started; only that commit makes the PR's branch a fair base. + const onAPr = Boolean(pr && String(prBranch ?? "").trim() && onReviewedCommit) + const job = String(req?.workflow ?? "").trim() || "change" + return { + head: req?.branch, + base: onAPr ? prBranch : defaultBranch || req?.base, + title: onAPr ? `Test Companion ${job} for #${pr}` : req?.title, + body: onAPr ? `${String(req?.body ?? "").trim()}\n\nRequested on #${pr}.`.trim() : req?.body, + draft: true, + } +} + +/** + * One commit, on `base`, with the branch's changes except `paths` (put back as they were + * at `base`). Its parent is `base`, not `tip`, so the left-out files are not in what is + * pushed. Built in a scratch index so the checkout is untouched. + * + * `git(args, extraEnv, input)` runs git; it must apply literal pathspecs. + * Returns { sha }, { empty } when nothing else changed, or { error }. + */ +export function leaveOut({ git, base, tip, paths, identity, message, indexFile }) { + const scratch = { GIT_INDEX_FILE: indexFile, GIT_LITERAL_PATHSPECS: "1" } + if (!git(["read-tree", tip], scratch).ok) return { error: "could not read the branch" } + if (paths.length) { + // Resetting to `base` restores a changed file and removes an added one. Paths on stdin: + // no argument-length limit, and no name read as an option. --no-refresh: don't re-read + // the checkout, which would run its clean filters. + const resetWith = (extra) => + git(["reset", "-q", ...extra, base, "--pathspec-from-file=-", "--pathspec-file-nul"], scratch, paths.join("\0")) + let reset = resetWith(["--no-refresh"]) + // --no-refresh needs git 2.36; an older self-hosted runner rejects the option. + if (!reset.ok && /unknown option|usage: git reset/i.test(String(reset.err ?? ""))) reset = resetWith([]) + if (!reset.ok) return { error: "could not leave out the by-products" } + } + const tree = git(["write-tree"], scratch) + if (!tree.ok || !tree.out) return { error: "could not write the branch's files" } + if (tree.out === git(["rev-parse", `${base}^{tree}`]).out) return { empty: true } + const commit = git( + ["-c", `user.name=${identity.name}`, "-c", `user.email=${identity.email}`, "commit-tree", tree.out, "-p", base, "-m", message], + scratch, + ) + return commit.ok && commit.out ? { sha: commit.out } : { error: "could not commit without the by-products" } +} + +// Agent-chosen text on one line, so it can't add a trailer such as Co-authored-by. +const oneLine = (s) => String(s ?? "").replace(/[\x00-\x1f\x7f]+/g, " ").trim() + +/** The commit message for a hand-off: its title, and any files left out. */ +export function leaveOutMessage(title, paths = []) { + const head = oneLine(title) || "Test Companion changes" + if (!paths.length) return head + return `${head}\n\nLeft out, because Test Companion never commits them:\n${listFiles(paths, { render: (p) => `- ${oneLine(p)}`, separator: "\n" })}` +} + +/** + * What a hand-off branch push means. Only an ordinary rejection says the branch is already + * there; `[remote rejected]` (a protection rule, a missing permission) means nothing was pushed. + */ +export function classifyHandoffPush({ ok, err } = {}) { + if (ok) return "pushed" + return /!\s*\[rejected\][^\n]*\((non-fast-forward|fetch first)\)/i.test(String(err ?? "")) ? "already" : "failed" +} + +/** + * The line of a failed `git push` that says why. git's last line is always the generic + * "failed to push some refs"; the reason (a ruleset, a hook, a missing permission) is earlier. + */ +export function pushFailureReason(err) { + const lines = String(err ?? "") + .split("\n") + .map((l) => l.trim()) + .filter(Boolean) + return ( + lines.find((l) => /^!\s*\[remote rejected\]/i.test(l)) ?? + lines.find((l) => /^remote:\s*(error|GH\d+)/i.test(l)) ?? + lines.find((l) => /^fatal:/i.test(l)) ?? + lines.at(-1) ?? + "" + ) +} + +/** The pull request an earlier run opened for this branch, from a state=all listing. */ +export function existingHandoffPr(prs) { + // A failed lookup is not "no pull request": treating it so could reopen closed work. + if (!Array.isArray(prs)) return { kind: "unknown" } + const list = prs.filter(Boolean) + const open = list.find((p) => p.state === "open") + if (open) return { kind: "open", url: open.html_url } + const closed = list.find((p) => p.state === "closed") + if (closed) return { kind: closed.merged_at ? "merged" : "closed", url: closed.html_url } + return { kind: "none" } +} + +/** Next step after the push: stop, report an existing pull request, or open one. */ +export function nextHandoffStep(pushKind, existing = { kind: "none" }) { + if (pushKind === "failed") return { action: "stop" } + if (pushKind === "pushed") return { action: "create", earlier: false } + if (existing.kind === "unknown") return { action: "stop", unknown: true } + if (existing.kind === "none") return { action: "create", earlier: true } + // A closed or merged one is reported, never reopened. + return { action: "report", ...existing } +} + +/** Did creating the pull request fail only because drafts aren't available on this plan? */ +export function draftRejected(result) { + return Boolean(result) && result.status !== 0 && /draft/i.test(`${result.stdout ?? ""}${result.stderr ?? ""}`) +} + +/** The comment line for a hand-off that did not end in a new pull request of this run's work. */ +export function handoffReport(step, { branch } = {}) { + if (step.action === "report" && step.kind === "open") { + return `**Already done:** this job's pull request is already open: ${step.url}.` + } + if (step.action === "report") { + return `**Already done:** an earlier run's pull request for this commit was ${step.kind}: ${step.url}. Push a new commit to run this job again.` + } + if (step.unknown) { + return `**Did not open a pull request:** ${codeSpan(branch)} already exists from an earlier run, and Test Companion could not check whether that run's pull request was closed. Look for it, or push a new commit to run this job again.` + } + return `**Could not push ${codeSpan(branch)}.**` +} diff --git a/test-companion/src/report.mjs b/test-companion/src/report.mjs index 7764d19..bb80907 100644 --- a/test-companion/src/report.mjs +++ b/test-companion/src/report.mjs @@ -1,6 +1,7 @@ /** Rendering and decisions for the run's comment, check and job summary. No network calls. */ import { CAPABILITIES, isApprovalOnly } from "./intent.mjs" import { emojiFor } from "./outcome.mjs" +import { ANSWER_JOBS } from "./gate.mjs" const withMark = (outcome) => { const mark = emojiFor(outcome) @@ -13,9 +14,15 @@ const withMark = (outcome) => { */ export const isOurs = (comment, us) => Boolean(us) && comment?.user?.type === "Bot" && comment?.user?.login === us -// Hidden marker so later runs edit one comment instead of posting new ones. +// Hidden marker on the one status comment automatic runs (no request comment) keep up to date. export const COMMENT_MARKER = "" +/** Marks the reply to one request, so a re-run of the same job edits it and nothing else does. */ +export function requestMarkerFor(requestId) { + const id = String(requestId ?? "").replace(/\D/g, "") + return id ? `` : COMMENT_MARKER +} + const previewRun = (summary) => summary?.preview === "true" // Flags generated-but-unsaved cases loudly; bare counts read as success. @@ -85,11 +92,20 @@ export function renderJobSummary({ workNote, extraChecks = [], warnings = [], + errorLine, + buildNote, + planWorkflow, } = {}) { const lines = [] lines.push(`## Test Companion — ${withMark(outcome)}`) lines.push("") + const why = oneLine(errorLine) + if (why) { + lines.push(`> **Why:** ${inertMarkers(why)}`) + lines.push("") + } + if (String(approvalNote ?? "").trim()) { lines.push(`>${inertMarkers(approvalNote)}`) lines.push("") @@ -107,10 +123,19 @@ export function renderJobSummary({ } } + if (String(buildNote ?? "").trim()) { + lines.push(`> ${inertMarkers(oneLine(buildNote))}`) + lines.push("") + } + if (previewRun(summary)) { - // Preview blocks Test Companion's own write tools, not shell commands the agent runs. + // Explain and find-the-bad-PR always run this way on purpose; the banner reads as "nothing happened". + // Decided by the job asked for, never by the agent-written summary. lines.push( - "> **Preview** — Test Companion refused every one of its own write tools, so it created, changed and saved nothing itself. Shell commands the agent ran are not covered by that.", + ANSWER_JOBS.has(planWorkflow) + ? "> Read-only: this job explains what happened; nothing it does is saved." + : // Preview blocks Test Companion's own write tools, not shell commands the agent runs. + "> **Preview** — Test Companion refused every one of its own write tools, so it created, changed and saved nothing itself. Shell commands the agent ran are not covered by that.", ) lines.push("") } @@ -165,8 +190,9 @@ function cutShortNote(runUrl) { return `\n\n_Cut short to fit a comment. The full version is on ${where}._` } -export function renderComment(args) { - return renderWithin(args, COMMENT_LIMIT, `\n\n${COMMENT_MARKER}`) +/** `marker` is requestMarkerFor(id) for a reply to a request; the shared status marker otherwise. */ +export function renderComment(args, marker = COMMENT_MARKER) { + return renderWithin(args, COMMENT_LIMIT, `\n\n${marker}`) } // GitHub's cap on a check run's summary; over it the check is never completed. @@ -198,9 +224,13 @@ function renderWithin(args, limit, tail) { return rest.slice(0, Math.max(0, limit - tail.length - note.length)) + note + tail } -// Only our own marked comment; a person quoting it must not capture the thread. -export function chooseCommentAction(comments, us) { - const ours = (comments ?? []).filter((c) => isOurs(c, us) && (c.body ?? "").includes(COMMENT_MARKER)) +/** + * Only our own marked comment; a person quoting it must not capture the thread. With a + * `requestId`, only that request's own reply is ever edited: one reply per request. + */ +export function chooseCommentAction(comments, us, { requestId } = {}) { + const marker = requestId ? requestMarkerFor(requestId) : COMMENT_MARKER + const ours = (comments ?? []).filter((c) => isOurs(c, us) && (c.body ?? "").includes(marker)) if (ours.length === 0) return { action: "create" } const oldest = ours.reduce((a, b) => (a.id <= b.id ? a : b)) return { action: "update", id: oldest.id } @@ -286,16 +316,79 @@ export function proposalAlreadyUsed(comments, sha, us, workflow) { return undefined } +/** + * Is this plan comment spent? Returns the marker that spent it, or undefined. + * + * A marker naming a plan always spends that plan. An approval ("consumed") also spends every + * other plan of the same job on the commit that already existed when it was recorded (posted + * before the comment carrying the marker), older or newer than the approved one: one approval + * settles that job on that commit, so neither a second `go` nor the nightly sweep acts on the + * others. Plans posted after it stay approvable. Dropping an expired plan spends only it and + * older plans of its job, never a newer plan that may still be waiting. A marker naming no job + * spends only its named plan. An older marker without a plan id spends the plans of its job + * (or every job, if it names none) posted up to the comment carrying it. + */ +function planSpentBy(planComment, marks) { + const id = Number(planComment?.id) + const job = readMarker(planComment?.body)?.workflow ?? "" + return marks.find((m) => { + if (m.plan) { + const named = Number(m.plan) + if (id === named) return true + if (!m.workflow || m.workflow !== job) return false + return m.kind === "consumed" ? id < Number(m.host) : id < named + } + const sameJob = !m.workflow || m.workflow === job + return sameJob && id <= Number(m.host) + }) +} + +/** The reply to `go #` when that plan can't be approved; `spentAs` is from planSpentState. */ +export function unavailablePlanReply(spentAs, id, sha) { + const plan = `Plan \`#${String(id ?? "").replace(/\D/g, "")}\`` + if (spentAs === "superseded") { + return ( + "A different plan for this job on this commit was already approved, so this one no longer applies. " + + "Push a new commit or ask again to get a fresh plan." + ) + } + if (spentAs === "approved" || spentAs === "dropped") { + return ( + `${plan} was already ${spentAs === "approved" ? "approved and run" : "dropped"}, so there is nothing left to approve on it. ` + + "Reply **go** on its own to approve the newest plan, or ask me again for a fresh one." + ) + } + return ( + `I have no plan \`#${String(id ?? "").replace(/\D/g, "")}\` still waiting on \`${String(sha ?? "").slice(0, 7)}\`. ` + + "It may have been written against an earlier commit. Reply **go** on its own to approve the most recent one." + ) +} + +/** + * "approved" or "dropped" when the plan comment `id` itself was approved or dropped, + * "superseded" when another plan of its job on the commit was approved instead, else undefined. + */ +export function planSpentState(comments, id, us) { + const plan = (comments ?? []).find((c) => isOurs(c, us) && String(c.id) === String(id) && readMarker(c.body)) + if (!plan) return undefined + const marks = spentPlans(comments, us).filter((m) => m.sha === readMarker(plan.body).sha) + // Its own fate first: a marker naming this plan says exactly what happened to it. + const own = marks.find((m) => m.plan && Number(m.plan) === Number(plan.id)) + const hit = own ?? planSpentBy(plan, marks) + if (!hit) return undefined + if (hit.kind === "dropped") return "dropped" + return hit.plan && Number(hit.plan) !== Number(plan.id) ? "superseded" : "approved" +} + + // Only our own comments count; an unreadable timestamp counts as expired. export function findProposal(comments, sha, now = Date.now(), us, opts = {}) { const want = String(sha ?? "").trim() - // Spent plans are tracked per job, so using one leaves the others on this commit waiting. + // Spent per plan, so approving one leaves a newer plan (same job or not) waiting. const spent = spentPlans(comments, us).filter((m) => m.sha === want) - if (spent.some((m) => !m.workflow)) return undefined - const spentJobs = new Set(spent.map((m) => m.workflow)) let ours = (comments ?? []).filter((c) => isOurs(c, us) && readMarker(c.body)?.sha === want) - ours = ours.filter((c) => !spentJobs.has(readMarker(c.body)?.workflow ?? "")) + ours = ours.filter((c) => !planSpentBy(c, spent)) // `go ` picks one plan when several are waiting. const named = String(opts?.id ?? "").trim() if (named) ours = ours.filter((c) => String(c.id) === named) @@ -334,12 +427,13 @@ export function renderApprovedPlan(proposalBody, replyText) { } // Spent-plan markers, so neither a second `go` nor the nightly sweep acts on a finished plan. -export function proposalConsumedMarkerFor(sha, workflow) { - return `` +// `planId` is the plan comment spent; without it the marker covers older plans (see planSpentBy). +export function proposalConsumedMarkerFor(sha, workflow, planId) { + return `` } -export function proposalDroppedMarkerFor(sha, workflow) { - return `` +export function proposalDroppedMarkerFor(sha, workflow, planId) { + return `` } function jobField(workflow) { @@ -347,15 +441,20 @@ function jobField(workflow) { return job ? ` workflow=${job}` : "" } -// A marker without `workflow=` covers every plan on the commit. -const SPENT_MARKER = //g +function planField(planId) { + const id = String(planId ?? "").replace(/\D/g, "") + return id ? ` plan=${id}` : "" +} + +const SPENT_MARKER = + //g function spentPlans(comments, us) { const out = [] for (const c of comments ?? []) { if (!isOurs(c, us)) continue for (const m of String(c.body ?? "").matchAll(SPENT_MARKER)) { - out.push({ kind: m[1], sha: m[2], workflow: m[3] }) + out.push({ kind: m[1], sha: m[2], workflow: m[3], plan: m[4], host: c.id }) } } return out @@ -457,3 +556,11 @@ export function inFlightCheckId(checks, checkName = "Test Companion") { if (ours.length === 0) return undefined return ours.reduce((a, b) => ((a.id ?? 0) >= (b.id ?? 0) ? a : b)).id } + +/** One line of at most 300 characters, for an error quoted in a comment. */ +function oneLine(text) { + const line = String(text ?? "") + .replace(/\s+/g, " ") + .trim() + return line.length > 300 ? `${line.slice(0, 299)}…` : line +} diff --git a/test-companion/src/runs.mjs b/test-companion/src/runs.mjs index e51ed40..36eb309 100644 --- a/test-companion/src/runs.mjs +++ b/test-companion/src/runs.mjs @@ -34,17 +34,180 @@ export function runIdsFromChecks(checks, { selfRunId, checkName = "Test Companio const RED = new Set(["failure", "cancelled", "timed_out", "startup_failure"]) -/** URL of the newest failed run on this commit, excluding our own runs. */ -export function latestFailedRun(runs, { selfRunId, selfWorkflowName } = {}) { - const red = (runs ?? []).filter( - (r) => - r && - r.status === "completed" && - RED.has(r.conclusion) && - r.html_url && - String(r.id) !== String(selfRunId) && - (!selfWorkflowName || r.name !== selfWorkflowName), - ) - if (red.length === 0) return undefined - return red.reduce((a, b) => ((a.id ?? 0) >= (b.id ?? 0) ? a : b)).html_url +const nameOf = (r) => + String(r?.name ?? "workflow") + .replace(/[[\]`<>|\r\n]/g, " ") + .trim() || "workflow" + +/** + * The other runs on a commit, newest first: finished red ones and ones still going. + * Excludes our own runs. + */ +export function ciOnCommit(runs, { selfRunId, selfWorkflowName } = {}) { + const others = (runs ?? []) + .filter((r) => r && String(r.id) !== String(selfRunId) && (!selfWorkflowName || r.name !== selfWorkflowName)) + .sort((a, b) => (b.id ?? 0) - (a.id ?? 0)) + const pick = (r) => ({ id: r.id, name: nameOf(r), url: r.html_url }) + return { + failed: others.filter((r) => r.status === "completed" && RED.has(r.conclusion) && r.html_url).map(pick), + running: others.filter((r) => IN_FLIGHT.has(r.status)).map(pick), + } +} + +/** + * A run the comment names explicitly: an Actions run link, or `run `. Returns { id }, + * { error } for a link to another host or repository, or undefined when none is named. + * Ids are digits only. + */ +export function parseRunReference(text, { repo, serverUrl } = {}) { + const s = String(text ?? "") + const link = /https?:\/\/([^/\s]+)\/([^/\s]+)\/([^/\s]+)\/actions\/runs\/(\d+)/i.exec(s) + if (link) { + let host = "github.com" + try { + host = new URL(serverUrl || "https://github.com").host + } catch { + // Default host. + } + const [, linkHost, owner, name, id] = link + if (linkHost.toLowerCase() !== host.toLowerCase() || `${owner}/${name}`.toLowerCase() !== String(repo ?? "").toLowerCase()) { + return { error: `That run link is not for ${repo || "this repository"}, so I can't use it.` } + } + return { id } + } + const bare = /\brun\s+#?(\d{6,})\b/i.exec(s) + return bare ? { id: bare[1] } : undefined +} + +// Words that say what to do, not which workflow to look at. +const NOT_A_NAME = new Set( + "browserstack explain fix find bad pr prs pull request the a an why what did does do failed failing fail failure build builds run runs this that these on in for please of to and go verdict rca me it is test tests ci check checks job jobs with my our".split( + " ", + ), +) + +const tokens = (text) => + String(text ?? "") + .toLowerCase() + .split(/[^a-z0-9]+/) + .filter(Boolean) + +// A matrix suffix ("E2E Tests (linux)") is not part of what people call the workflow. +const nameTokens = (name) => tokens(String(name ?? "").replace(/\([^)]*\)/g, " ")) +const significant = (words) => words.filter((w) => w.length >= 2 && !NOT_A_NAME.has(w)) + +/** + * The failed run a comment names by workflow (e.g. "explain e2e"). The whole workflow name, + * as words, wins (longest first, so "Lint Checks" beats "Lint"); otherwise every significant + * word of a name must appear, the run with the most such words wins, ties go to the newest. + * A name made only of everyday words ("build", "Test", "CI", "explain") never matches: the + * comment uses those words anyway. Such a run is asked for by `run `. + * `failed` is newest first. Returns undefined when no workflow is named. + */ +export function matchFailedByName(text, failed) { + const said = tokens(text) + const padded = ` ${said.join(" ")} ` + const runs = failed ?? [] + let best + let bestLen = 0 + for (const r of runs) { + const name = nameTokens(r.name) + if (significant(name).length && padded.includes(` ${name.join(" ")} `) && name.length > bestLen) { + best = r + bestLen = name.length + } + } + if (best) return best + const saidSet = new Set(said) + let most = 0 + for (const r of runs) { + const words = significant(nameTokens(r.name)) + if (words.length && words.every((w) => saidSet.has(w)) && words.length > most) { + best = r + most = words.length + } + } + return best +} + +/** The reply when GitHub could not be asked about the builds; never "nothing failed". */ +export const LOOKUP_FAILED_REPLY = + "I couldn't look up this pull request's builds on GitHub just now, so I can't tell which one failed. Please ask me again in a minute." + +/** + * A `gh api` result as found, not found (HTTP 404), or failed (anything else), so a lookup + * that went wrong is never read as "no such run" or "nothing failed". + */ +export function classifyLookup(result) { + if (result && result.status === 0) { + try { + return { found: JSON.parse(result.stdout) } + } catch { + return { failed: true } + } + } + return /HTTP 404|Not Found/i.test(`${result?.stderr ?? ""}${result?.stdout ?? ""}`) ? { notFound: true } : { failed: true } +} + +// GitHub lists at most this many commits of a pull request. +export const PR_COMMIT_LIST_LIMIT = 250 + +/** + * The pull request's commit ids from a `--paginate --slurp` listing, always including the + * head. `complete` is false when the listing failed or may have been cut off at GitHub's limit. + */ +export function prCommitShas(slurped, headSha) { + const listed = Array.isArray(slurped) + ? slurped + .flat() + .map((c) => c?.sha) + .filter(Boolean) + : [] + const shas = [...new Set([...listed, headSha].filter(Boolean))] + return { shas, complete: Array.isArray(slurped) && listed.length < PR_COMMIT_LIST_LIMIT } +} + +/** Why a run named in a comment can't be used, or { ok, url, name }. */ +export function checkNamedRun(run, { prShas = [], shasComplete = true, selfRunId, selfWorkflowName } = {}) { + if (!run?.id || !run?.html_url) return { why: "I could not find that run in this repository." } + if (String(run.id) === String(selfRunId) || (selfWorkflowName && run.name === selfWorkflowName)) { + return { why: "That is one of Test Companion's own runs." } + } + // Fails closed: a run with no commit id never matches. + if (!run.head_sha || !prShas.includes(run.head_sha)) { + return { + why: shasComplete + ? "That run is not for a commit on this pull request." + : "I couldn't list all of this pull request's commits, so I can only use runs on its latest commit. Name a run from the latest commit, or ask without naming one.", + } + } + if (IN_FLIGHT.has(run.status)) return { why: "That run is still going. Ask me again when it finishes." } + if (run.status !== "completed" || !RED.has(run.conclusion)) { + return { why: `That run did not fail (it finished as ${run.conclusion ?? "unknown"}).` } + } + return { ok: true, url: run.html_url, name: nameOf(run) } +} + +const listNames = (runs, max = 5) => { + // A code span: a workflow name can't mention anyone, link, or format (nameOf strips backticks). + const shown = runs.slice(0, max).map((r) => `\`${r.name}\``) + return runs.length > max ? `${shown.join(", ")} and ${runs.length - max} more` : shown.join(", ") +} + +/** The reply when nothing has failed yet but CI is still running on the commit. */ +export function renderCiStillRunning(sha, running, failed = []) { + const short = String(sha ?? "").slice(0, 7) + const done = failed.length ? ` Already failed: ${listNames(failed)}.` : "" + return `CI is still running on \`${short}\`: ${listNames(running)}.${done} Ask me again when it finishes, and I will look at whatever went red.` +} + +/** Which run was looked at, which other failures were not, and how to ask about one. */ +export function renderBuildNote(chosen, { otherFailed = [], running = [], ask = "explain" } = {}) { + if (!chosen) return "" + const parts = [`Looked at \`${chosen.name}\` ([run](${chosen.url})).`] + if (otherFailed.length) { + parts.push(`Also failed: ${listNames(otherFailed)}. To look at one, ask \`@browserstack ${ask} run ${otherFailed[0].id}\` or paste its run link.`) + } + if (running.length) parts.push(`Still running: ${listNames(running)}.`) + return parts.join(" ") } diff --git a/test-companion/src/sandbox.mjs b/test-companion/src/sandbox.mjs index eae47b6..b30c6d8 100644 --- a/test-companion/src/sandbox.mjs +++ b/test-companion/src/sandbox.mjs @@ -7,6 +7,7 @@ import { spawnSync } from "node:child_process" import { closeSync, constants, copyFileSync, existsSync, fstatSync, openSync, readSync, writeFileSync } from "node:fs" import { userInfo } from "node:os" import { AGENT_USER, LAUNCHER_SOURCE, sandboxLayout } from "./sandbox-policy.mjs" +import { BOT_EMAIL, BOT_NAME } from "./push-work.mjs" const SUDO = "/usr/bin/sudo" @@ -38,7 +39,7 @@ function must(r, what) { return r } -export function prepareSandbox({ repoDir, tcPath }) { +export function prepareSandbox({ repoDir, tcPath, identity = { name: BOT_NAME, email: BOT_EMAIL } }) { const layout = sandboxLayout() const me = userInfo().username @@ -90,7 +91,7 @@ export function prepareSandbox({ repoDir, tcPath }) { // safe.directory: git refuses a repository owned by another user. writeAs( `${layout.home}/.gitconfig`, - `[safe]\n\tdirectory = ${layout.work}\n[user]\n\tname = Test Companion\n\temail = test-companion[bot]@users.noreply.github.com\n`, + `[safe]\n\tdirectory = ${layout.work}\n[user]\n\tname = ${identity.name}\n\temail = ${identity.email}\n`, `${AGENT_USER}:${AGENT_USER}`, "644", ) diff --git a/test-companion/starter-workflow.yml b/test-companion/starter-workflow.yml index a157de4..37e534b 100644 --- a/test-companion/starter-workflow.yml +++ b/test-companion/starter-workflow.yml @@ -53,12 +53,14 @@ jobs: runs-on: ubuntu-latest timeout-minutes: 35 # A loose first filter so unrelated comments don't boot a runner; the Action makes - # the real decision. The last two lines catch a bare "go" approval (case-insensitive). + # the real decision. Bot comments, our own included, never start one. The last two + # lines catch a bare "go" approval (case-insensitive). if: >- github.event_name != 'issue_comment' - || contains(github.event.comment.body, '@browserstack') + || (github.event.comment.user.type != 'Bot' + && (contains(github.event.comment.body, '@browserstack') || startsWith(github.event.comment.body, 'go') - || startsWith(github.event.comment.body, 'approve') + || startsWith(github.event.comment.body, 'approve'))) # Keep this block even with the App: the workflow token is the fallback. permissions: contents: write # branches and commits for fix / reconcile jobs diff --git a/test-companion/test/answer.test.mjs b/test-companion/test/answer.test.mjs index 44a46e8..97c6ec9 100644 --- a/test-companion/test/answer.test.mjs +++ b/test-companion/test/answer.test.mjs @@ -1,6 +1,6 @@ import { test, describe } from "node:test" import assert from "node:assert/strict" -import { createAnswerCapture, runCapturingAnswer, MAX_PARTIAL_LINE } from "../src/answer.mjs" +import { createAnswerCapture, runCapturingAnswer, MAX_PARTIAL_LINE, redactSecrets, redactDeep, secretsFrom } from "../src/answer.mjs" // One `answer` event as the CLI prints it under --json. const said = (text) => JSON.stringify({ event: "answer", text }) + "\n" @@ -174,3 +174,109 @@ describe("runCapturingAnswer", () => { assert.equal(r.answer, undefined) }) }) + +describe("the CLI's reason for stopping is kept", () => { + const node = process.execPath + + test("from an `Error:` line on stderr", async () => { + const r = await runCapturingAnswer(node, ["-e", `process.stderr.write('Error: project "x" not found\\n'); process.exit(1)`], process.env) + assert.equal(r.error, 'project "x" not found') + }) + + test("from an error event on stdout", async () => { + const ev = JSON.stringify({ event: "error", fatal: true, message: "You've hit your Free credit limit." }) + const r = await runCapturingAnswer(node, ["-e", `process.stdout.write(${JSON.stringify(ev)} + "\\n"); process.exit(1)`], process.env) + assert.equal(r.error, "You've hit your Free credit limit.") + }) + + test("the CLI's structured error wins over a plain Error: line", async () => { + const ev = JSON.stringify({ event: "error", message: "You've hit your Free credit limit." }) + const r = await runCapturingAnswer( + node, + ["-e", `process.stdout.write(${JSON.stringify(ev)} + "\\n"); process.stderr.write("Error: task failed\\n"); process.exit(1)`], + process.env, + ) + assert.equal(r.error, "You've hit your Free credit limit.") + }) + + test("an Error: line in the agent's test output on stdout is not the reason", async () => { + const r = await runCapturingAnswer( + node, + ["-e", `process.stdout.write("Error: expect(received).toBe(expected)\\n"); process.stderr.write("Error: test failed\\n"); process.exit(1)`], + process.env, + ) + assert.equal(r.error, "test failed") + }) + + test("stdout alone with only test output gives no reason", async () => { + const r = await runCapturingAnswer(node, ["-e", `process.stdout.write("Error: boom\\n"); process.exit(1)`], process.env) + assert.equal(r.error, undefined) + }) + + test("a clean run has none", async () => { + const r = await runCapturingAnswer(node, ["-e", "process.exit(0)"], process.env) + assert.equal(r.error, undefined) + }) +}) + +describe("redactSecrets", () => { + test("known secrets are replaced", () => { + assert.equal(redactSecrets("key abc123secret in use", ["abc123secret"]), "key *** in use") + }) + + test("credentials in a URL are hidden even when not known", () => { + assert.equal( + redactSecrets("connect https://alice:s3cr3tkey@hub-cloud.browserstack.com/wd/hub failed"), + "connect https://***@hub-cloud.browserstack.com/wd/hub failed", + ) + }) + + test("a longer secret is replaced whole before a shorter one inside it", () => { + assert.equal(redactSecrets("tok=abcdef123456", ["abcdef", "abcdef123456"]), "tok=***") + }) + + test("short or empty values are not treated as secrets", () => { + assert.equal(redactSecrets("a b c", ["", "a", undefined]), "a b c") + }) + + test("nothing to redact passes through, and undefined stays undefined", () => { + assert.equal(redactSecrets("plain"), "plain") + assert.equal(redactSecrets(undefined), undefined) + }) +}) + +describe("secretsFrom", () => { + test("credential-named variables are collected, others are not", () => { + const got = secretsFrom({ + BROWSERSTACK_ACCESS_KEY: "k1", + GH_TOKEN: "t1", + TC_ACTION_APP_PRIVATE_KEY: "p1", + BROWSERSTACK_USERNAME: "alice", + HOME: "/home/runner", + }) + assert.deepEqual(got.sort(), ["k1", "p1", "t1"]) + }) +}) + +describe("redactDeep — every string in a payload sent to GitHub", () => { + test("hides secrets in nested comment, check and pull request payloads", () => { + const payload = { + body: "ran with key abc123secret", + output: { title: "t", summary: "https://u:p4ss@hub/x", annotations: [{ message: "abc123secret" }] }, + draft: true, + n: 3, + } + assert.deepEqual(redactDeep(payload, ["abc123secret"]), { + body: "ran with key ***", + output: { title: "t", summary: "https://***@hub/x", annotations: [{ message: "***" }] }, + draft: true, + n: 3, + }) + }) + + test("the original token stays hidden after the environment changes", () => { + const startup = secretsFrom({ GH_TOKEN: "ghs_workflowtoken1" }) + const later = secretsFrom({ GH_TOKEN: "ghs_apptoken22222" }) + assert.equal(redactDeep({ body: "ghs_workflowtoken1 ghs_apptoken22222" }, [...startup, ...later]).body, "*** ***") + }) +}) diff --git a/test-companion/test/outcome.test.mjs b/test-companion/test/outcome.test.mjs index 7ee60ba..28ba86a 100644 --- a/test-companion/test/outcome.test.mjs +++ b/test-companion/test/outcome.test.mjs @@ -1,6 +1,6 @@ import { test, describe } from "node:test" import assert from "node:assert/strict" -import { OUTCOMES, parseSummary, resolveOutcome, checkConclusion, stepShouldFail, approvalWasSpent } from "../src/outcome.mjs" +import { OUTCOMES, parseSummary, resolveOutcome, checkConclusion, stepShouldFail, approvalWasSpent, runDidNotWork } from "../src/outcome.mjs" describe("parseSummary", () => { test("reads the CLI's key: value report", () => { @@ -173,3 +173,14 @@ describe("approvalWasSpent", () => { } }) }) + +describe("runDidNotWork — there is nothing to approve", () => { + test("broke, never started, or hit a limit", () => { + for (const o of [OUTCOMES.RUN_BROKE, OUTCOMES.COULD_NOT_RUN, OUTCOMES.STOPPED_AT_LIMIT]) assert.equal(runDidNotWork(o), true) + }) + + test("succeeded or failed tests is a real result", () => { + assert.equal(runDidNotWork(OUTCOMES.SUCCEEDED), false) + assert.equal(runDidNotWork(OUTCOMES.TESTS_FAILED), false) + }) +}) diff --git a/test-companion/test/plan-approval.test.mjs b/test-companion/test/plan-approval.test.mjs new file mode 100644 index 0000000..39a9a56 --- /dev/null +++ b/test-companion/test/plan-approval.test.mjs @@ -0,0 +1,89 @@ +import { test, describe } from "node:test" +import assert from "node:assert/strict" +import { + findProposal, + planSpentState, + proposalConsumedMarkerFor, + proposalDroppedMarkerFor, + proposalMarkerFor, +} from "../src/report.mjs" + +const US = "browserstack[bot]" +const SHA = "abc1234" +const now = Date.parse("2026-11-12T12:00:00Z") +const fresh = "2026-11-12T11:00:00Z" +const plan = (id, workflow = "generate", extra = "") => ({ + id, + user: { type: "Bot", login: US }, + created_at: fresh, + html_url: `https://x/${id}`, + body: `the plan ${id}${extra}\n\n${proposalMarkerFor(SHA, "alice", workflow)}`, +}) +const ours = (id, body) => ({ id, user: { type: "Bot", login: US }, created_at: fresh, body }) + +describe("go approves the newest unspent plan", () => { + test("a spent plan for the same job and commit doesn't block a newer one", () => { + const comments = [plan(10, "generate", `\n${proposalConsumedMarkerFor(SHA, "generate", 10)}`), plan(20)] + assert.equal(findProposal(comments, SHA, now, US)?.id, 20) + }) + + test("the spent plan itself is not approvable again", () => { + const comments = [plan(10, "generate", `\n${proposalConsumedMarkerFor(SHA, "generate", 10)}`)] + assert.equal(findProposal(comments, SHA, now, US), undefined) + assert.equal(findProposal(comments, SHA, now, US, { id: 10 }), undefined) + }) + + test("an approval settles the job's plans that existed then; one posted afterwards stays open", () => { + const comments = [plan(10), plan(20), ours(30, `report\n${proposalConsumedMarkerFor(SHA, "generate", 10)}`), plan(40)] + assert.equal(findProposal(comments, SHA, now, US)?.id, 40) + assert.equal(findProposal(comments, SHA, now, US, { id: 20 }), undefined) + assert.equal(findProposal(comments, SHA, now, US, { id: 40 })?.id, 40) + }) + + test("an approval of one job leaves another job's plans open", () => { + const comments = [plan(10), plan(20, "fix"), ours(30, `report\n${proposalConsumedMarkerFor(SHA, "generate", 10)}`)] + assert.equal(findProposal(comments, SHA, now, US)?.id, 20) + }) + + test("an old marker without a plan id spends only plans posted before it", () => { + const comments = [plan(10), ours(15, `ran it\n${proposalConsumedMarkerFor(SHA, "generate")}`), plan(20)] + assert.equal(findProposal(comments, SHA, now, US)?.id, 20) + assert.equal(findProposal(comments, SHA, now, US, { id: 10 }), undefined) + }) + + test("an old marker without a job still covers earlier plans of every job", () => { + const comments = [plan(10, "fix"), ours(15, proposalConsumedMarkerFor(SHA)), plan(20, "fix")] + assert.equal(findProposal(comments, SHA, now, US, { id: 10 }), undefined) + assert.equal(findProposal(comments, SHA, now, US)?.id, 20) + }) + + test("a dropped marker with a plan id drops only that plan", () => { + const comments = [plan(10), ours(12, `dropped\n${proposalDroppedMarkerFor(SHA, "generate", 10)}`), plan(20)] + assert.equal(findProposal(comments, SHA, now, US)?.id, 20) + }) + + test("a marker another account posted counts for nothing", () => { + const forged = { id: 25, user: { type: "Bot", login: "other[bot]" }, body: proposalConsumedMarkerFor(SHA, "generate", 20) } + assert.equal(findProposal([plan(20), forged], SHA, now, US)?.id, 20) + }) +}) + +describe("planSpentState", () => { + test("says whether a named plan was approved or dropped", () => { + const comments = [ + plan(10, "generate", `\n${proposalConsumedMarkerFor(SHA, "generate", 10)}`), + plan(20), + ours(22, proposalDroppedMarkerFor(SHA, "generate", 20)), + plan(30), + ] + assert.equal(planSpentState(comments, 10, US), "approved") + assert.equal(planSpentState(comments, "20", US), "dropped") + assert.equal(planSpentState(comments, 30, US), undefined) + assert.equal(planSpentState(comments, 99, US), undefined) + }) + + test("markers carry only digits for the plan id", () => { + assert.match(proposalConsumedMarkerFor(SHA, "fix", "12x3"), / plan=123 -->$/) + assert.doesNotMatch(proposalConsumedMarkerFor(SHA, "fix"), /plan=/) + }) +}) diff --git a/test-companion/test/plan-run.test.mjs b/test-companion/test/plan-run.test.mjs index a9917d8..63ce06b 100644 --- a/test-companion/test/plan-run.test.mjs +++ b/test-companion/test/plan-run.test.mjs @@ -636,3 +636,24 @@ describe("the @browserstack mention", () => { assert.match(out.context.instructions, /test this PR/) }) }) + +describe("comments from bots, our own included, are ignored", () => { + const fromUser = (user) => ({ + action: "created", + issue: { number: 7, pull_request: {} }, + comment: { body: "Reply **go** to run this, or **@browserstack go** with your edits.", user }, + }) + const run = (user) => planRun({ eventName: "issue_comment", repo: "acme/shop", payload: fromUser(user) }) + + test("the App's own comment", () => { + assert.equal(run({ login: "browserstack[bot]", type: "Bot" }).skip?.reason, "bot-comment") + }) + + test("a bot login without a type", () => { + assert.equal(run({ login: "github-actions[bot]" }).skip?.reason, "bot-comment") + }) + + test("a person saying the same thing still counts", () => { + assert.equal(run({ login: "an-sdet", type: "User" }).skip, undefined) + }) +}) diff --git a/test-companion/test/push-work.test.mjs b/test-companion/test/push-work.test.mjs index d07c6eb..736ccc8 100644 --- a/test-companion/test/push-work.test.mjs +++ b/test-companion/test/push-work.test.mjs @@ -1,13 +1,14 @@ import { test, describe } from "node:test" import assert from "node:assert/strict" -import { execFileSync } from "node:child_process" -import { mkdtempSync, rmSync, readdirSync, readFileSync } from "node:fs" +import { execFileSync, spawnSync } from "node:child_process" +import { mkdtempSync, mkdirSync, rmSync, readdirSync, readFileSync, writeFileSync } from "node:fs" import { tmpdir } from "node:os" -import { join } from "node:path" +import { dirname, join } from "node:path" import { shouldPushWork, changedPaths, splitPushable, + splitHandoffPaths, renderPushNote, authoredByUs, needsCheckout, @@ -23,6 +24,20 @@ import { GIT_SAFE_ARGS, pushUrlFor, BOT_EMAIL, + handedOffPrRequest, + commitIdentity, + savingFor, + leaveOut, + leaveOutMessage, + classifyHandoffPush, + pushFailureReason, + existingHandoffPr, + nextHandoffStep, + draftRejected, + handoffReport, + codeSpan, + listFiles, + APP_SLUGS, } from "../src/push-work.mjs" const live = { preview: false, sameRepo: true, branch: "feat/x", headAuthorEmail: "dev@example.com", onWantedCommit: true } @@ -645,3 +660,390 @@ describe("splitPushable refuses .github and symlinks", () => { assert.deepEqual(skip, ["tests/fixtures/link"]) }) }) + +describe("jobs that don't produce code never push", () => { + const live = { preview: false, sameRepo: true, branch: "feature/x", onWantedCommit: true, defaultBranch: "main" } + + for (const workflow of ["generate", "rca", "verdict", "validate"]) { + test(`${workflow} saves nothing to the branch`, () => { + const out = shouldPushWork({ ...live, workflow }) + assert.equal(out.ok, false) + assert.match(out.why, /does not change code/) + }) + } + + test("a job that writes code still may", () => { + assert.equal(shouldPushWork({ ...live, workflow: "test-pr" }).ok, true) + assert.equal(shouldPushWork({ ...live, workflow: "fix" }).ok, true) + }) +}) + +describe("BrowserStack tooling's by-products are never committed", () => { + test("browserstack.err and .browserstack files are skipped", () => { + const { push, skip } = splitPushable(["browserstack.err", ".browserstack-local.json", "src/a.test.ts", "nested/browserstack.err"]) + assert.deepEqual(push, ["src/a.test.ts"]) + assert.deepEqual(skip, ["browserstack.err", ".browserstack-local.json", "nested/browserstack.err"]) + }) +}) + +describe("handedOffPrRequest — a fix asked for on a pull request targets that pull request", () => { + const req = { workflow: "fix", branch: "tc/fix/abc", base: "main", title: "tc fix: abc", body: "What changed." } + + test("on a pull request: base is its branch, a draft, a title that names it", () => { + const out = handedOffPrRequest({ req, pr: "42", prBranch: "feat/cart", defaultBranch: "main" }) + assert.equal(out.base, "feat/cart") + assert.equal(out.head, "tc/fix/abc") + assert.equal(out.draft, true) + assert.equal(out.title, "Test Companion fix for #42") + assert.match(out.body, /Requested on #42\./) + }) + + test("the title names the job from the hand-off, not the plan", () => { + const out = handedOffPrRequest({ req: { ...req, workflow: "automate" }, pr: "42", prBranch: "feat/cart" }) + assert.equal(out.title, "Test Companion automate for #42") + }) + + test("if the run didn't start on the pull request's commit, the default branch is the base", () => { + const out = handedOffPrRequest({ req, pr: "42", prBranch: "feat/cart", defaultBranch: "main", onReviewedCommit: false }) + assert.equal(out.base, "main") + }) + + test("without a pull request: the default branch, the CLI's own title", () => { + const out = handedOffPrRequest({ req, defaultBranch: "main" }) + assert.equal(out.base, "main") + assert.equal(out.title, "tc fix: abc") + assert.equal(out.draft, true) + }) + + test("no default branch known: the CLI's base", () => { + assert.equal(handedOffPrRequest({ req, workflow: "automate" }).base, "main") + }) +}) + +describe("commitIdentity — commits are credited to the App when acting as it", () => { + test("the App's bot account, with the noreply email GitHub links to it", () => { + assert.deepEqual(commitIdentity({ appSlug: "browserstack", appUserId: "355331011" }), { + name: "browserstack[bot]", + email: "355331011+browserstack[bot]@users.noreply.github.com", + }) + }) + + test("without the App, or without its id, the fixed identity", () => { + assert.deepEqual(commitIdentity(), { name: "Test Companion", email: BOT_EMAIL }) + assert.deepEqual(commitIdentity({ appSlug: "browserstack", appUserId: "" }), { name: "Test Companion", email: BOT_EMAIL }) + assert.deepEqual(commitIdentity({ appSlug: "browserstack", appUserId: "not-a-number" }), { name: "Test Companion", email: BOT_EMAIL }) + }) + + test("the loop guard knows the App's commits are ours too", () => { + const app = "355331011+browserstack[bot]@users.noreply.github.com" + assert.equal(authoredByUs(app, [app]), true) + assert.equal(authoredByUs(BOT_EMAIL), true) + assert.equal(authoredByUs("dev@example.com", [app]), false) + assert.equal(authoredByUs("", [""]), false) + }) +}) + +describe("splitHandoffPaths — a handed-back branch", () => { + test("tool by-products are dropped, not a reason to refuse", () => { + const { refused, dropped } = splitHandoffPaths(["src/a.ts", "browserstack.err", ".browserstack-cache/x", "run.log"]) + assert.deepEqual(refused, []) + assert.deepEqual(dropped, ["browserstack.err", ".browserstack-cache/x", "run.log"]) + }) + + test(".github files and symlinks still refuse the branch", () => { + const { refused, dropped } = splitHandoffPaths([".github/workflows/x.yml", "link", "browserstack.err"], { + isSymlink: (p) => p === "link", + }) + assert.deepEqual(refused, [".github/workflows/x.yml", "link"]) + assert.deepEqual(dropped, ["browserstack.err"]) + }) + + test("ordinary files are neither refused nor dropped", () => { + assert.deepEqual(splitHandoffPaths(["src/a.ts", "tests/a.spec.ts"]), { refused: [], dropped: [] }) + }) +}) + +// A real git, with the Action's own environment, for the git-level tests below. +function realGit(cwd) { + const env = gitEnvFrom(process.env) + return (args, extraEnv = {}, input = undefined) => { + const r = spawnSync("git", args, { cwd, encoding: "utf8", input, env: { ...env, ...extraEnv } }) + return { ok: r.status === 0, out: (r.stdout ?? "").trim(), err: (r.stderr ?? "").trim() } + } +} + +function repoWithBranch(files) { + const dir = mkdtempSync(join(tmpdir(), "tc-leaveout-")) + const git = realGit(dir) + const id = ["-c", "user.name=t", "-c", "user.email=t@example.com"] + const must = (args) => { + const r = git(args) + assert.ok(r.ok, `${args.join(" ")}: ${r.err}`) + return r.out + } + must(["init", "-q"]) + writeFileSync(join(dir, "a.ts"), "old\n") + must(["add", "a.ts"]) + must([...id, "commit", "-qm", "base"]) + const base = must(["rev-parse", "HEAD"]) + for (const [name, content] of Object.entries(files)) { + mkdirSync(dirname(join(dir, name)), { recursive: true }) + writeFileSync(join(dir, name), content) + } + must(["add", "--", ...Object.keys(files)]) + must([...id, "commit", "-qm", "work"]) + return { dir, git, base, tip: must(["rev-parse", "HEAD"]) } +} + +describe("leaveOut — by-products never reach the pushed branch", () => { + const identity = { name: "Test Companion", email: BOT_EMAIL } + + test("one commit on the base: the work survives, the by-product is in no pushed commit", () => { + const { dir, git, base, tip } = repoWithBranch({ "a.ts": "new\n", "browserstack.err": "[object Object]", ".env": "KEY=secret" }) + const out = leaveOut({ git, base, tip, paths: ["browserstack.err", ".env"], identity, message: "m", indexFile: join(dir, "scratch") }) + assert.ok(out.sha, JSON.stringify(out)) + // Only the new commit between base and the head: the agent's commit is not reachable. + assert.deepEqual(git(["rev-list", `${base}..${out.sha}`]).out.split("\n"), [out.sha]) + assert.equal(git(["diff", "--name-only", base, out.sha]).out, "a.ts") + assert.equal(git(["show", `${out.sha}:a.ts`]).out, "new") + assert.equal(git(["cat-file", "-e", `${out.sha}:browserstack.err`]).ok, false) + assert.equal(git(["cat-file", "-e", `${out.sha}:.env`]).ok, false) + rmSync(dir, { recursive: true, force: true }) + }) + + test("a file named like pathspec magic is treated as a file name", () => { + const { dir, git, base, tip } = repoWithBranch({ "a.ts": "new\n", ":!keep/.env": "KEY=secret" }) + const out = leaveOut({ git, base, tip, paths: [":!keep/.env"], identity, message: "m", indexFile: join(dir, "scratch") }) + assert.ok(out.sha, JSON.stringify(out)) + assert.equal(git(["show", `${out.sha}:a.ts`]).out, "new", "the work was reset along with the by-product") + assert.equal(git(["diff", "--name-only", base, out.sha]).out, "a.ts") + rmSync(dir, { recursive: true, force: true }) + }) + + test("when the by-products were the only changes, it says so and makes no commit", () => { + const { dir, git, base, tip } = repoWithBranch({ "browserstack.err": "[object Object]" }) + assert.deepEqual( + leaveOut({ git, base, tip, paths: ["browserstack.err"], identity, message: "m", indexFile: join(dir, "scratch") }), + { empty: true }, + ) + rmSync(dir, { recursive: true, force: true }) + }) + + test("the commit message names the job and the left-out files, capped", () => { + const many = Array.from({ length: 12 }, (_, i) => `log/${i}.log`) + const msg = leaveOutMessage("Fix the cart total", many) + assert.match(msg, /^Fix the cart total\n\nLeft out/) + assert.match(msg, /and 2 more$/) + }) +}) + +describe("leaveOut always squashes the agent's history", () => { + const identity = { name: "Test Companion", email: BOT_EMAIL } + + test("a .env added then deleted in the agent's commits is in no pushed commit, and the author is ours", () => { + const { dir, git, base } = repoWithBranch({ "a.ts": "new\n", ".env": "KEY=secret" }) + const id = ["-c", "user.name=agent", "-c", "user.email=agent@example.com"] + assert.ok(git(["rm", "-q", "--", ".env"]).ok) + assert.ok(git([...id, "commit", "-qm", "drop env"]).ok) + const tip = git(["rev-parse", "HEAD"]).out + // Nothing to leave out in the final tree, but the history still holds .env. + const out = leaveOut({ git, base, tip, paths: [], identity, message: "m", indexFile: join(dir, "scratch") }) + assert.ok(out.sha, JSON.stringify(out)) + const range = git(["rev-list", `${base}..${out.sha}`]).out.split("\n") + assert.deepEqual(range, [out.sha]) + for (const c of range) assert.equal(git(["cat-file", "-e", `${c}:.env`]).ok, false) + assert.equal(git(["log", "-1", "--format=%an <%ae>", out.sha]).out, `Test Companion <${BOT_EMAIL}>`) + assert.equal(git(["show", `${out.sha}:a.ts`]).out, "new") + rmSync(dir, { recursive: true, force: true }) + }) + + test("a branch with no net change is reported as empty", () => { + const { dir, git, base } = repoWithBranch({ "a.ts": "new\n" }) + writeFileSync(join(dir, "a.ts"), "old\n") + assert.ok(git(["-c", "user.name=agent", "-c", "user.email=agent@example.com", "commit", "-qam", "undo"]).ok) + const tip = git(["rev-parse", "HEAD"]).out + assert.deepEqual(leaveOut({ git, base, tip, paths: [], identity, message: "m", indexFile: join(dir, "scratch") }), { empty: true }) + rmSync(dir, { recursive: true, force: true }) + }) + + test("an agent-chosen file name can't add a trailer to the commit message", () => { + const msg = leaveOutMessage("Fix\n\nCo-authored-by: x ", ["x\n\nCo-authored-by: Someone "]) + assert.equal(msg.split("\n").some((line) => /^co-authored-by:/i.test(line)), false) + assert.match(msg, /^Fix Co-authored-by: x \n/) + }) + + test("with nothing left out, the message is just the title", () => { + assert.equal(leaveOutMessage("Fix the cart total", []), "Fix the cart total") + }) +}) + +describe("handing back a pull request: what happened, and what to do next", () => { + test("an ordinary rejection means the branch is already there", () => { + const err = "To https://github.com/o/r.git\n ! [rejected] abc -> tc/fix/abc (non-fast-forward)\nerror: failed to push" + assert.equal(classifyHandoffPush({ ok: false, err }), "already") + assert.equal(classifyHandoffPush({ ok: false, err: " ! [rejected] abc -> b (fetch first)" }), "already") + }) + + test("a remote rejection (protection rule, missing permission) means nothing was pushed", () => { + const err = " ! [remote rejected] abc -> tc/fix/abc (refusing to allow a GitHub App to create or update workflow)" + assert.equal(classifyHandoffPush({ ok: false, err }), "failed") + assert.equal(classifyHandoffPush({ ok: false, err: "fatal: unable to access" }), "failed") + assert.equal(classifyHandoffPush({ ok: true }), "pushed") + }) + + test("an open pull request wins; a closed or merged one is reported, never reopened", () => { + const open = { state: "open", html_url: "https://x/1" } + const closed = { state: "closed", html_url: "https://x/2", merged_at: null } + const merged = { state: "closed", html_url: "https://x/3", merged_at: "2026-01-01" } + assert.deepEqual(existingHandoffPr([closed, open]), { kind: "open", url: "https://x/1" }) + assert.deepEqual(existingHandoffPr([closed]), { kind: "closed", url: "https://x/2" }) + assert.deepEqual(existingHandoffPr([merged]), { kind: "merged", url: "https://x/3" }) + assert.deepEqual(existingHandoffPr([]), { kind: "none" }) + }) + + test("next step for each push outcome", () => { + assert.deepEqual(nextHandoffStep("failed"), { action: "stop" }) + assert.deepEqual(nextHandoffStep("pushed"), { action: "create", earlier: false }) + assert.deepEqual(nextHandoffStep("already", { kind: "none" }), { action: "create", earlier: true }) + assert.deepEqual(nextHandoffStep("already", { kind: "closed", url: "u" }), { action: "report", kind: "closed", url: "u" }) + assert.deepEqual(nextHandoffStep("already", { kind: "open", url: "u" }), { action: "report", kind: "open", url: "u" }) + }) + + test("the report never claims a push or reopens closed work", () => { + assert.match(handoffReport({ action: "report", kind: "open", url: "u" }), /^\*\*Already done:\*\* .*already open: u\.$/) + const closed = handoffReport({ action: "report", kind: "closed", url: "u" }) + assert.match(closed, /was closed: u\. Push a new commit/) + assert.match(handoffReport({ action: "report", kind: "merged", url: "u" }), /was merged: u\./) + const stop = handoffReport({ action: "stop" }, { branch: "tc/fix/abc" }) + assert.match(stop, /^\*\*Could not push `tc\/fix\/abc`\.\*\*$/) + assert.doesNotMatch(stop, /Pushed/) + }) + + test("a draft rejection is recognised; other failures are not", () => { + assert.equal(draftRejected({ status: 1, stderr: "Draft pull requests are not supported in this repository" }), true) + assert.equal(draftRejected({ status: 1, stderr: "Validation Failed: No commits between" }), false) + assert.equal(draftRejected({ status: 0, stdout: "{}" }), false) + }) +}) + +describe("git reads file names literally", () => { + test("the Action's git environment turns pathspec magic off", () => { + assert.equal(gitEnvFrom({}).GIT_LITERAL_PATHSPECS, "1") + }) + + test("adding a file named `:!x` stages only that file", () => { + const dir = mkdtempSync(join(tmpdir(), "tc-literal-")) + const git = realGit(dir) + assert.ok(git(["init", "-q"]).ok) + writeFileSync(join(dir, ":!x"), "1") + writeFileSync(join(dir, ".env"), "KEY=secret") + assert.ok(git(["add", "--", ":!x"]).ok) + assert.equal(git(["diff", "--cached", "--name-only"]).out, ":!x") + rmSync(dir, { recursive: true, force: true }) + }) +}) + +describe("savingFor — the job the pipeline chose still decides", () => { + test("an event run that turned out to be generate saves nothing, quietly", () => { + const out = savingFor({ ok: true }, "generate") + assert.equal(out.ok, false) + assert.equal(out.quiet, true) + }) + + test("a job that writes code keeps the earlier decision", () => { + assert.deepEqual(savingFor({ ok: true }, "test-pr"), { ok: true }) + assert.deepEqual(savingFor({ ok: false, why: "x" }, undefined), { ok: false, why: "x" }) + }) + + test("shouldPushWork marks the no-code refusal quiet", () => { + assert.equal(shouldPushWork({ ...live, workflow: "rca" }).quiet, true) + assert.equal(shouldPushWork({ ...live, preview: true }).quiet, undefined) + }) +}) + +describe("authoredByUs — the App's commits are ours whatever this run is", () => { + test("the App's noreply email is recognised without this run's identity", () => { + assert.equal(authoredByUs("355331011+browserstack[bot]@users.noreply.github.com"), true) + assert.ok(APP_SLUGS.includes("browserstack")) + }) + + test("another app's bot is not ours", () => { + assert.equal(authoredByUs("49699333+dependabot[bot]@users.noreply.github.com"), false) + }) + + test("this run's own identity, compared trimmed and in any case", () => { + assert.equal(authoredByUs("Me@Example.com", [" me@example.com "]), true) + assert.equal(authoredByUs("", [""]), false) + }) +}) + +describe("file names in comments", () => { + test("a plain name is ordinary code", () => { + assert.equal(codeSpan("src/a.ts"), "`src/a.ts`") + }) + + test("a backtick in the name can't break out of the code span", () => { + assert.equal(codeSpan("a`b"), "`` a`b ``") + assert.equal(codeSpan("x``y"), "``` x``y ```") + }) + + test("newlines in a name are flattened", () => { + assert.equal(codeSpan("a\nb"), "`a b`") + }) + + test("lists stop at 10 names", () => { + const names = Array.from({ length: 13 }, (_, i) => `f${i}`) + const out = listFiles(names) + assert.equal(out.split(", ").length, 11) + assert.match(out, /and 3 more$/) + }) +}) + +describe("hand-off: failed lookups and push reasons", () => { + test("a failed PR lookup is unknown, and stops rather than opening a pull request", () => { + assert.deepEqual(existingHandoffPr(null), { kind: "unknown" }) + assert.deepEqual(existingHandoffPr({ message: "Not Found" }), { kind: "unknown" }) + const step = nextHandoffStep("already", existingHandoffPr(null)) + assert.deepEqual(step, { action: "stop", unknown: true }) + assert.match(handoffReport(step, { branch: "tc/fix/abc" }), /could not check whether that run's pull request was closed/) + }) + + test("a server-side refusal reports the rule, not git's generic last line", () => { + const err = [ + "remote: error: GH013: Repository rule violations found for refs/heads/tc/fix/abc.", + "To https://github.com/o/r.git", + " ! [remote rejected] abc -> tc/fix/abc (push declined due to repository rule violations)", + "error: failed to push some refs to 'https://github.com/o/r.git'", + ].join("\n") + assert.match(pushFailureReason(err), /^! \[remote rejected\].*repository rule violations/) + }) + + test("a hook refusal without [remote rejected] falls back to the remote error line", () => { + const err = "remote: error: commits must be signed\nerror: failed to push some refs to 'x'" + assert.equal(pushFailureReason(err), "remote: error: commits must be signed") + }) + + test("fatal lines and plain last lines are used when nothing better is there", () => { + assert.equal(pushFailureReason("fatal: unable to access 'x': 403\n"), "fatal: unable to access 'x': 403") + assert.equal(pushFailureReason("something odd"), "something odd") + assert.equal(pushFailureReason(""), "") + }) +}) + +describe("leaveOut on a git without --no-refresh", () => { + test("retries the reset without the option", () => { + const calls = [] + const git = (args, _env, input) => { + calls.push(args) + if (args[0] === "reset" && args.includes("--no-refresh")) return { ok: false, err: "error: unknown option `no-refresh'" } + if (args[0] === "write-tree") return { ok: true, out: "tree2" } + if (args[0] === "rev-parse") return { ok: true, out: "tree1" } + if (args.includes("commit-tree")) return { ok: true, out: "sha2" } + return { ok: true, out: "" } + } + const r = leaveOut({ git, base: "b", tip: "t", paths: ["browserstack.err"], identity: { name: "n", email: "e" }, message: "m", indexFile: "/tmp/x" }) + assert.deepEqual(r, { sha: "sha2" }) + assert.equal(calls.filter((a) => a[0] === "reset").length, 2) + assert.ok(!calls.filter((a) => a[0] === "reset")[1].includes("--no-refresh")) + }) +}) diff --git a/test-companion/test/reply-per-request.test.mjs b/test-companion/test/reply-per-request.test.mjs new file mode 100644 index 0000000..f14f5f9 --- /dev/null +++ b/test-companion/test/reply-per-request.test.mjs @@ -0,0 +1,91 @@ +import { test, describe } from "node:test" +import assert from "node:assert/strict" +import { chooseCommentAction, requestMarkerFor, renderComment, renderJobSummary, COMMENT_MARKER } from "../src/report.mjs" +import { shouldPushWork } from "../src/push-work.mjs" + +const US = "browserstack[bot]" +const bot = (id, body) => ({ id, body, user: { type: "Bot", login: US } }) + +describe("one reply per request", () => { + test("a request gets a new reply, never the shared status comment or another request's", () => { + const comments = [bot(1, `status\n\n${COMMENT_MARKER}`), bot(2, `answer\n\n${requestMarkerFor(500)}`)] + assert.deepEqual(chooseCommentAction(comments, US, { requestId: 600 }), { action: "create" }) + }) + + test("a re-run of the same request edits its own reply", () => { + const comments = [bot(1, `status\n\n${COMMENT_MARKER}`), bot(2, `answer\n\n${requestMarkerFor(500)}`)] + assert.deepEqual(chooseCommentAction(comments, US, { requestId: 500 }), { action: "update", id: 2 }) + }) + + test("automatic runs keep one status comment, which a request reply never takes over", () => { + assert.deepEqual(chooseCommentAction([bot(2, `answer\n\n${requestMarkerFor(500)}`)], US), { action: "create" }) + assert.deepEqual(chooseCommentAction([bot(1, `status\n\n${COMMENT_MARKER}`)], US), { action: "update", id: 1 }) + }) + + test("the reply ends with its request's marker", () => { + const body = renderComment({ outcome: "Succeeded", summary: { workflow: "rca" } }, requestMarkerFor(500)) + assert.ok(body.endsWith(requestMarkerFor(500))) + assert.ok(!body.includes(COMMENT_MARKER)) + }) + + test("a request marker takes digits only; no id falls back to the status marker", () => { + assert.equal(requestMarkerFor("12") + assert.equal(requestMarkerFor(undefined), COMMENT_MARKER) + }) + + test("agent text can't forge a request marker", () => { + const body = renderComment({ outcome: "Succeeded", answer: `see ${requestMarkerFor(500)}` }, requestMarkerFor(600)) + assert.equal(body.split(requestMarkerFor(500)).length - 1, 0) + }) +}) + +describe("the preview banner", () => { + test("read-only jobs say so instead of the preview banner", () => { + for (const workflow of ["rca", "verdict"]) { + const body = renderJobSummary({ workflow, planWorkflow: workflow, outcome: "Succeeded", summary: { preview: "true" } }) + assert.match(body, /Read-only: this job explains what happened; nothing it does is saved/) + assert.doesNotMatch(body, /\*\*Preview\*\*/) + } + }) + + test("a real preview of a job that writes keeps the banner", () => { + const body = renderJobSummary({ workflow: "generate", outcome: "Succeeded", summary: { preview: "true" } }) + assert.match(body, /\*\*Preview\*\*/) + }) + + test("a live run shows neither", () => { + const body = renderJobSummary({ workflow: "fix", outcome: "Succeeded", summary: { preview: "false" } }) + assert.doesNotMatch(body, /Preview|Read-only/) + }) +}) + +describe("the build note", () => { + test("is shown, on one line, with markers neutralised", () => { + const body = renderJobSummary({ outcome: "Succeeded", buildNote: "Looked at **E2E**.\n" }) + assert.match(body, /> Looked at \*\*E2E\*\*\. <!-- test-companion-run -->/) + }) +}) + +describe("own-commit guard", () => { + const base = { + sameRepo: true, + branch: "feature", + onWantedCommit: true, + defaultBranch: "main", + headAuthorEmail: "355331011+browserstack[bot]@users.noreply.github.com", + ourEmails: ["355331011+browserstack[bot]@users.noreply.github.com"], + workflow: "fix", + } + test("an automatic run still won't push on top of our own commit", () => { + assert.deepEqual(shouldPushWork(base), { ok: false, why: "this run is reacting to our own commit" }) + }) + test("a run a person asked for or approved may", () => { + assert.deepEqual(shouldPushWork({ ...base, requestedByPerson: true }), { ok: true }) + }) + test("asking doesn't bypass any other refusal", () => { + assert.equal(shouldPushWork({ ...base, requestedByPerson: true, branch: "main" }).ok, false) + assert.equal(shouldPushWork({ ...base, requestedByPerson: true, sameRepo: false }).ok, false) + assert.equal(shouldPushWork({ ...base, requestedByPerson: true, preview: true }).ok, false) + assert.equal(shouldPushWork({ ...base, requestedByPerson: true, workflow: "generate" }).ok, false) + }) +}) diff --git a/test-companion/test/report.test.mjs b/test-companion/test/report.test.mjs index 1d8b016..7dfe007 100644 --- a/test-companion/test/report.test.mjs +++ b/test-companion/test/report.test.mjs @@ -1244,3 +1244,20 @@ describe("a plan cannot choose which job its approval runs", () => { assert.equal(isApprovableJob("generate", undefined), false) }) }) + +describe("a run that didn't work says why", () => { + test("the error line leads the comment, on one line", () => { + const md = renderJobSummary({ ...base, outcome: OUTCOMES.RUN_BROKE, errorLine: 'project "rag_assignment"\nnot found' }) + assert.match(md, /> \*\*Why:\*\* project "rag_assignment" not found/) + }) + + test("a very long reason is cut", () => { + const md = renderJobSummary({ ...base, outcome: OUTCOMES.RUN_BROKE, errorLine: "x".repeat(1000) }) + const why = md.split("\n").find((l) => l.startsWith("> **Why:**")) + assert.ok(why.length < 330) + }) + + test("no error line, no Why", () => { + assert.doesNotMatch(renderJobSummary(base), /\*\*Why:\*\*/) + }) +}) diff --git a/test-companion/test/review-round-fixes.test.mjs b/test-companion/test/review-round-fixes.test.mjs new file mode 100644 index 0000000..df149e3 --- /dev/null +++ b/test-companion/test/review-round-fixes.test.mjs @@ -0,0 +1,295 @@ +import { test, describe } from "node:test" +import assert from "node:assert/strict" +import { + findProposal, + planSpentState, + proposalNeedsDropping, + proposalConsumedMarkerFor, + proposalDroppedMarkerFor, + proposalMarkerFor, + unavailablePlanReply, + renderJobSummary, +} from "../src/report.mjs" +import { + matchFailedByName, + renderBuildNote, + renderCiStillRunning, + checkNamedRun, + classifyLookup, + prCommitShas, + parseRunReference, + PR_COMMIT_LIST_LIMIT, +} from "../src/runs.mjs" +import { isRequestedByPerson } from "../src/push-work.mjs" +import { readIntent } from "../src/intent.mjs" + +const US = "browserstack[bot]" +const SHA = "abc1234" +const now = Date.parse("2026-11-12T12:00:00Z") +const fresh = "2026-11-12T11:00:00Z" +const stale = "2026-11-10T11:00:00Z" +const plan = (id, workflow = "fix", { created = fresh, extra = "" } = {}) => ({ + id, + user: { type: "Bot", login: US }, + created_at: created, + html_url: `https://x/${id}`, + body: `the plan ${id}${extra}\n\n${proposalMarkerFor(SHA, "alice", workflow)}`, +}) +const ours = (id, body) => ({ id, user: { type: "Bot", login: US }, created_at: fresh, body }) + +describe("approving a newer plan also spends the older ones of that job", () => { + // Two plans for the same job on one commit; the newer one was approved. + const comments = (created = fresh) => [ + plan(100, "fix", { created }), + plan(200, "fix", { created }), + ours(300, `run report\n${proposalConsumedMarkerFor(SHA, "fix", 200)}`), + ] + + test("a bare go finds no waiting plan, so the older plan can't be run", () => { + assert.equal(findProposal(comments(), SHA, now, US), undefined) + }) + + test("the nightly sweep has nothing to drop, so it never flips the check", () => { + assert.equal(proposalNeedsDropping(comments(stale), SHA, now, US), false) + }) + + test("go # is told another plan was approved instead, not that it ran", () => { + assert.equal(planSpentState(comments(), 100, US), "superseded") + assert.equal(planSpentState(comments(), 200, US), "approved") + }) + + test("approving the older plan also settles a newer one that was already waiting", () => { + const c = (created = fresh) => [ + plan(100, "fix", { created }), + plan(200, "fix", { created }), + ours(300, proposalConsumedMarkerFor(SHA, "fix", 100)), + ] + assert.equal(findProposal(c(), SHA, now, US), undefined) + assert.equal(planSpentState(c(), 200, US), "superseded") + assert.equal(planSpentState(c(), 100, US), "approved") + // So the sweep never drops the newer plan and flips the check over the approved run. + assert.equal(proposalNeedsDropping(c(stale), SHA, now, US), false) + }) + + test("a plan posted after the approval stays approvable", () => { + const c = [plan(100), ours(300, proposalConsumedMarkerFor(SHA, "fix", 100)), plan(400)] + assert.equal(findProposal(c, SHA, now, US)?.id, 400) + assert.equal(planSpentState(c, 400, US), undefined) + }) + + test("a marker that names a plan but no job spends only that plan", () => { + const c = [plan(100, "fix"), plan(150, "generate"), plan(200, "fix"), ours(300, proposalConsumedMarkerFor(SHA, "", 200))] + assert.equal(planSpentState(c, 200, US), "approved") + assert.equal(planSpentState(c, 100, US), undefined) + assert.equal(planSpentState(c, 150, US), undefined) + assert.equal(findProposal(c, SHA, now, US)?.id, 150) + }) + + test("an approval on one commit leaves plans on another commit alone", () => { + const other = { ...plan(250, "fix"), body: `p\n\n${proposalMarkerFor("def5678", "alice", "fix")}` } + const c = [plan(200, "fix"), other, ours(300, proposalConsumedMarkerFor(SHA, "fix", 200))] + assert.equal(planSpentState(c, 250, US), undefined) + assert.equal(findProposal(c, "def5678", now, US)?.id, 250) + }) + + test("dropping an expired plan never spends a newer one still waiting", () => { + const c = [ + plan(100, "fix", { created: stale }), + plan(200, "fix"), + ours(300, proposalDroppedMarkerFor(SHA, "fix", 100)), + ] + assert.equal(planSpentState(c, 100, US), "dropped") + assert.equal(planSpentState(c, 200, US), undefined) + assert.equal(findProposal(c, SHA, now, US)?.id, 200) + }) + + test("an old marker without a plan id keeps its old meaning", () => { + const c = [plan(100), plan(200), ours(300, proposalConsumedMarkerFor(SHA, "fix")), plan(400)] + assert.equal(planSpentState(c, 100, US), "approved") + assert.equal(planSpentState(c, 200, US), "approved") + assert.equal(findProposal(c, SHA, now, US)?.id, 400) + }) + + test("an older plan for a different job stays open", () => { + const c = [plan(100, "generate"), plan(200, "fix"), ours(300, proposalConsumedMarkerFor(SHA, "fix", 200))] + assert.equal(findProposal(c, SHA, now, US)?.id, 100) + }) +}) + +describe("naming a failed workflow in the comment", () => { + const failed = [ + { id: 9, name: "Lint" }, + { id: 8, name: "CI" }, + { id: 7, name: "Build and Test" }, + { id: 6, name: "Lint Checks" }, + { id: 5, name: "Deploy main" }, + { id: 4, name: "e2e" }, + ] + const pick = (text) => matchFailedByName(text, failed)?.name + + test("a whole workflow name wins, longest first", () => { + assert.equal(pick("@browserstack explain Lint Checks"), "Lint Checks") + assert.equal(pick("@browserstack explain lint"), "Lint") + assert.equal(pick("@browserstack explain e2e please"), "e2e") + }) + + test("a name made only of everyday words never matches; it is asked for by run id", () => { + // The comment uses these words anyway ("why this build failed"), so they can't name a run. + assert.equal(pick("@browserstack explain CI"), undefined) + assert.equal(pick("@browserstack explain Build and Test"), undefined) + }) + + test("an incidental word does not name a workflow", () => { + assert.equal(pick("@browserstack explain the failure on main"), undefined) + assert.equal(pick("@browserstack explain why the build failed"), undefined) + assert.equal(pick("@browserstack explain"), undefined) + }) + + test("every significant word of a name counts as naming it", () => { + assert.equal(pick("@browserstack explain the main deploy"), "Deploy main") + }) +}) + +describe("everyday-word workflow names and matrix suffixes", () => { + const pick = (text, failed) => matchFailedByName(text, failed)?.name + + test("a workflow called build, Test or explain is not picked by a sentence that uses the word", () => { + assert.equal(pick("explain why this build failed", [{ id: 9, name: "e2e" }, { id: 8, name: "build" }]), undefined) + assert.equal(pick("fix the failing test", [{ id: 9, name: "Lint" }, { id: 8, name: "Test" }]), undefined) + assert.equal(pick("explain", [{ id: 9, name: "Lint" }, { id: 8, name: "explain" }]), undefined) + assert.equal(pick("explain CI / build", [{ id: 9, name: "e2e" }, { id: 8, name: "CI / build" }]), undefined) + }) + + test("a matrix suffix doesn't stop the workflow from being named", () => { + assert.equal(pick("explain e2e", [{ id: 9, name: "Lint" }, { id: 8, name: "E2E Tests (linux)" }]), "E2E Tests (linux)") + assert.equal(pick("explain e2e", [{ id: 9, name: "Lint" }, { id: 8, name: "e2e (chrome, 18)" }]), "e2e (chrome, 18)") + }) + + test("between two matrix legs, ties go to the newest", () => { + assert.equal(pick("explain e2e", [{ id: 9, name: "e2e (linux)" }, { id: 8, name: "e2e (mac)" }]), "e2e (linux)") + }) +}) + +describe("the reply to go # on a plan that can't be approved", () => { + test("superseded says another plan was approved, not that this one ran", () => { + const text = unavailablePlanReply("superseded", 100, SHA) + assert.match(text, /different plan for this job on this commit was already approved/) + assert.doesNotMatch(text, /approved and run/) + }) + + test("approved, dropped and unknown keep their own replies", () => { + assert.match(unavailablePlanReply("approved", 100, SHA), /was already approved and run/) + assert.match(unavailablePlanReply("dropped", 100, SHA), /was already dropped/) + assert.match(unavailablePlanReply(undefined, 100, SHA), /I have no plan `#100` still waiting on `abc1234`/) + }) + + test("the id is digits only in the reply", () => { + assert.match(unavailablePlanReply("approved", "1`0@x", SHA), /Plan `#10`/) + }) +}) + +describe("the 'also failed' hint asks by run id", () => { + const note = renderBuildNote( + { id: 9, name: "Lint", url: "https://github.com/o/r/actions/runs/9" }, + { otherFailed: [{ id: 12345678, name: "CI", url: "u" }], ask: "explain" }, + ) + + test("the hint names the run by id, which the parser and intent reader understand", () => { + assert.match(note, /`@browserstack explain run 12345678`/) + assert.deepEqual(parseRunReference("@browserstack explain run 12345678", { repo: "o/r" }), { id: "12345678" }) + assert.equal(readIntent("explain run 12345678", { workflows: ["rca", "fix"] }).workflow, "rca") + }) +}) + +describe("workflow names in replies are inert", () => { + const hostile = { id: 1, name: "x @org/core #12 *y* https://e.example", url: "https://github.com/o/r/actions/runs/1" } + + test("names are code spans, so they can't mention, link or format", () => { + const note = renderBuildNote(hostile, { otherFailed: [hostile], running: [hostile] }) + assert.ok(note.includes("`x @org/core #12 *y* https://e.example`")) + assert.doesNotMatch(note, /\*\*x @org/) + const running = renderCiStillRunning("abcdef0123", [hostile], [hostile]) + assert.ok(running.includes("`x @org/core #12 *y* https://e.example`")) + }) +}) + +describe("isRequestedByPerson", () => { + test("a pull request comment is a person's request (bots are filtered earlier)", () => { + assert.equal(isRequestedByPerson({ isPrComment: true }), true) + }) + + test("a manual run counts only when a human account started it", () => { + assert.equal(isRequestedByPerson({ eventName: "workflow_dispatch", sender: { type: "User", login: "alice" } }), true) + assert.equal( + isRequestedByPerson({ eventName: "workflow_dispatch", sender: { type: "Bot", login: "browserstack[bot]" } }), + false, + ) + assert.equal( + isRequestedByPerson({ eventName: "workflow_dispatch", sender: { type: "User", login: "github-actions[bot]" } }), + false, + ) + assert.equal(isRequestedByPerson({ eventName: "workflow_dispatch" }), false) + }) + + test("automatic events never count", () => { + for (const eventName of ["push", "pull_request", "schedule", "workflow_run"]) { + assert.equal(isRequestedByPerson({ eventName, sender: { type: "User", login: "alice" } }), false) + } + }) +}) + +describe("run lookups fail closed and say so", () => { + test("a 404 is not found; any other failure is a failed lookup", () => { + assert.deepEqual(classifyLookup({ status: 0, stdout: '{"id":1}' }), { found: { id: 1 } }) + assert.deepEqual(classifyLookup({ status: 1, stderr: "gh: Not Found (HTTP 404)" }), { notFound: true }) + assert.deepEqual(classifyLookup({ status: 1, stderr: "gh: Server Error (HTTP 502)" }), { failed: true }) + assert.deepEqual(classifyLookup({ status: 0, stdout: "not json" }), { failed: true }) + assert.deepEqual(classifyLookup(undefined), { failed: true }) + }) + + test("commit ids drop missing values and always include the head", () => { + const got = prCommitShas([[{ sha: "a" }, {}, null, { sha: undefined }], [{ sha: "b" }]], "h") + assert.deepEqual(got, { shas: ["a", "b", "h"], complete: true }) + }) + + test("a failed listing keeps only the head and is marked incomplete", () => { + assert.deepEqual(prCommitShas(null, "h"), { shas: ["h"], complete: false }) + }) + + test("a listing cut off at GitHub's limit is marked incomplete", () => { + const page = Array.from({ length: PR_COMMIT_LIST_LIMIT }, (_, i) => ({ sha: `s${i}` })) + assert.equal(prCommitShas([page], "h").complete, false) + }) + + test("a run with no commit id never matches, even if the list held a gap", () => { + const run = { id: 5, html_url: "u", status: "completed", conclusion: "failure" } + assert.ok(checkNamedRun(run, { prShas: prCommitShas([[{}]], "h").shas }).why) + }) + + test("a run on an unlisted commit of a too-long pull request says why", () => { + const run = { id: 5, html_url: "u", head_sha: "old", status: "completed", conclusion: "failure" } + assert.match(checkNamedRun(run, { prShas: ["h"], shasComplete: false }).why, /couldn't list all/) + assert.match(checkNamedRun(run, { prShas: ["h"], shasComplete: true }).why, /not for a commit on this pull request/) + }) +}) + +describe("the read-only line follows the job that was asked for", () => { + const summary = { preview: "true" } + + test("explain shows the read-only line", () => { + const md = renderJobSummary({ workflow: "rca", planWorkflow: "rca", outcome: "Succeeded", summary }) + assert.match(md, /Read-only: this job explains what happened; nothing it does is saved\./) + assert.doesNotMatch(md, /\*\*Preview\*\*/) + }) + + test("an agent-written summary can't swap the preview banner for the read-only line", () => { + const md = renderJobSummary({ workflow: "rca", outcome: "Succeeded", summary: { ...summary, workflow: "rca" } }) + assert.match(md, /\*\*Preview\*\*/) + assert.doesNotMatch(md, /Read-only/) + }) + + test("a preview of a job that writes keeps the banner", () => { + const md = renderJobSummary({ workflow: "fix", planWorkflow: "fix", outcome: "Succeeded", summary }) + assert.match(md, /\*\*Preview\*\*/) + }) +}) diff --git a/test-companion/test/run-selection.test.mjs b/test-companion/test/run-selection.test.mjs new file mode 100644 index 0000000..233a837 --- /dev/null +++ b/test-companion/test/run-selection.test.mjs @@ -0,0 +1,137 @@ +import { test, describe } from "node:test" +import assert from "node:assert/strict" +import { + ciOnCommit, + parseRunReference, + matchFailedByName, + checkNamedRun, + renderCiStillRunning, + renderBuildNote, +} from "../src/runs.mjs" + +const run = (id, name, status, conclusion = null, extra = {}) => ({ + id, + name, + status, + conclusion, + html_url: `https://github.com/o/r/actions/runs/${id}`, + head_sha: "abc1234", + ...extra, +}) + +describe("ciOnCommit", () => { + test("splits red and still-running runs, newest first, leaving out our own", () => { + const ci = ciOnCommit( + [ + run(1, "Unit tests", "completed", "failure"), + run(3, "E2E", "completed", "failure"), + run(2, "Lint", "completed", "success"), + run(4, "Deploy", "in_progress"), + run(5, "Test Companion", "in_progress"), + run(6, "Other", "completed", "failure"), + ], + { selfRunId: 6, selfWorkflowName: "Test Companion" }, + ) + assert.deepEqual( + ci.failed.map((r) => r.name), + ["E2E", "Unit tests"], + ) + assert.deepEqual( + ci.running.map((r) => r.name), + ["Deploy"], + ) + }) + + test("a workflow name can't break the comment's formatting", () => { + const ci = ciOnCommit([run(1, "x`] (evil)|\nname", "completed", "failure")]) + assert.doesNotMatch(ci.failed[0].name, /[`[\]|\n]/) + }) +}) + +describe("parseRunReference", () => { + const opts = { repo: "Org/Repo", serverUrl: "https://github.com" } + + test("a run link for this repository, with or without a job", () => { + assert.deepEqual(parseRunReference("explain https://github.com/org/repo/actions/runs/123", opts), { id: "123" }) + assert.deepEqual(parseRunReference("fix https://github.com/Org/Repo/actions/runs/456/job/789", opts), { id: "456" }) + }) + + test("a link to another repository or host is refused", () => { + assert.ok(parseRunReference("explain https://github.com/evil/repo/actions/runs/1", opts).error) + assert.ok(parseRunReference("explain https://example.com/org/repo/actions/runs/1", opts).error) + }) + + test("`run ` with a long enough number", () => { + assert.deepEqual(parseRunReference("explain run 43189383014", opts), { id: "43189383014" }) + assert.deepEqual(parseRunReference("explain run #43189383014", opts), { id: "43189383014" }) + assert.equal(parseRunReference("explain run 12", opts), undefined) + }) + + test("nothing named, including a plan id in a go", () => { + assert.equal(parseRunReference("explain", opts), undefined) + assert.equal(parseRunReference("go #6824956824", opts), undefined) + assert.equal(parseRunReference(undefined, opts), undefined) + }) + + test("a GitHub Enterprise host is matched from the server URL", () => { + const ghe = { repo: "org/repo", serverUrl: "https://ghe.example.com" } + assert.deepEqual(parseRunReference("https://ghe.example.com/org/repo/actions/runs/9", ghe), { id: "9" }) + assert.ok(parseRunReference("https://github.com/org/repo/actions/runs/9", ghe).error) + }) +}) + +describe("matchFailedByName", () => { + const failed = [ + { id: 3, name: "E2E Tests", url: "u3" }, + { id: 1, name: "Unit tests", url: "u1" }, + ] + test("a word from the workflow name picks that run", () => { + assert.equal(matchFailedByName("explain e2e", failed).id, 3) + assert.equal(matchFailedByName("why did unit fail", failed).id, 1) + }) + test("job words and filler never pick a run", () => { + assert.equal(matchFailedByName("explain the failing tests please", failed), undefined) + assert.equal(matchFailedByName("browserstack fix", failed), undefined) + assert.equal(matchFailedByName("", failed), undefined) + }) +}) + +describe("checkNamedRun", () => { + const prShas = ["abc1234", "def5678"] + test("a finished red run on one of the PR's commits is used", () => { + const r = checkNamedRun(run(9, "E2E", "completed", "failure"), { prShas }) + assert.deepEqual(r, { ok: true, url: "https://github.com/o/r/actions/runs/9", name: "E2E" }) + }) + test("each reason a run can't be used", () => { + assert.match(checkNamedRun(null, { prShas }).why, /could not find/) + assert.match(checkNamedRun(run(9, "E2E", "completed", "failure", { head_sha: "zzz" }), { prShas }).why, /not for a commit/) + assert.match(checkNamedRun(run(9, "E2E", "in_progress"), { prShas }).why, /still going/) + assert.match(checkNamedRun(run(9, "E2E", "completed", "success"), { prShas }).why, /did not fail/) + assert.match(checkNamedRun(run(9, "TC", "completed", "failure"), { prShas, selfWorkflowName: "TC" }).why, /own runs/) + }) +}) + +describe("the replies", () => { + test("CI still running lists what is going", () => { + const text = renderCiStillRunning("abcdef1234", [{ name: "Unit tests" }, { name: "E2E" }]) + assert.match(text, /CI is still running on `abcdef1`: `Unit tests`, `E2E`/) + assert.match(text, /Ask me again when it finishes/) + }) + test("CI still running caps the list at five", () => { + const many = Array.from({ length: 7 }, (_, i) => ({ name: `w${i}` })) + assert.match(renderCiStillRunning("abc", many), /and 2 more/) + }) + test("the build note says which run was used and what else failed", () => { + const note = renderBuildNote( + { name: "E2E", url: "https://x/1" }, + { otherFailed: [{ id: 4242424, name: "Unit tests" }], running: [{ name: "Deploy" }], ask: "explain" }, + ) + assert.match(note, /Looked at `E2E` \(\[run\]\(https:\/\/x\/1\)\)/) + assert.match(note, /Also failed: `Unit tests`\. To look at one, ask `@browserstack explain run 4242424`/) + assert.match(note, /Still running: `Deploy`/) + }) + test("no other failures, no extra text", () => { + assert.equal(renderBuildNote({ name: "E2E", url: "u" }), "Looked at `E2E` ([run](u)).") + assert.equal(renderBuildNote(undefined), "") + }) +}) diff --git a/test-companion/test/runs.test.mjs b/test-companion/test/runs.test.mjs index 6a9e155..b5eba12 100644 --- a/test-companion/test/runs.test.mjs +++ b/test-companion/test/runs.test.mjs @@ -1,6 +1,6 @@ import { test, describe } from "node:test" import assert from "node:assert/strict" -import { chooseRunsToCancel, runIdsFromChecks, latestFailedRun } from "../src/runs.mjs" +import { chooseRunsToCancel, runIdsFromChecks, ciOnCommit } from "../src/runs.mjs" const inProgress = (id, over = {}) => ({ id, @@ -90,7 +90,9 @@ describe("runIdsFromChecks — finding OUR in-flight runs on a commit", () => { }) }) -describe("latestFailedRun — which build 'explain the failure' means", () => { +describe("ciOnCommit — which build 'explain the failure' means", () => { + // The newest failed run is failed[0]. + const latestFailedRun = (runs, opts) => ciOnCommit(runs, opts).failed[0]?.url // `rca`, `fix` and `verdict` need to know which build failed. const run = (id, conclusion, name = "CI", status = "completed") => ({ id, diff --git a/test-companion/test/starter.test.mjs b/test-companion/test/starter.test.mjs index 18dfc98..7269383 100644 --- a/test-companion/test/starter.test.mjs +++ b/test-companion/test/starter.test.mjs @@ -200,6 +200,86 @@ describe("a comment that is not for us does not start a job", () => { assert.match(job, /startsWith\(github\.event\.comment\.body, 'approve'\)/) }) + // Evaluates the job's `if:` the way GitHub does, for the operators it uses. + const condition = job + .slice(job.indexOf("if: >-") + "if: >-".length, job.indexOf("\n #", job.indexOf("if: >-"))) + .replace(/\s+/g, " ") + .trim() + function evaluate(expr, ctx) { + const tokens = expr.match(/'[^']*'|\|\||&&|!=|==|[(),]|[A-Za-z_][\w.]*/g) + let i = 0 + const peek = () => tokens[i] + const take = () => tokens[i++] + const value = (path) => path.split(".").reduce((o, k) => (o == null ? undefined : o[k]), ctx) + const lower = (v) => String(v ?? "").toLowerCase() + function primary() { + const t = take() + if (t === "(") { + const v = or() + take() + return v + } + if (t.startsWith("'")) return t.slice(1, -1) + if (peek() === "(") { + take() + const a = or() + take() + const b = or() + take() + if (t === "contains") return lower(a).includes(lower(b)) + if (t === "startsWith") return lower(a).startsWith(lower(b)) + throw new Error(`unknown function ${t}`) + } + return value(t) + } + function compare() { + const a = primary() + if (peek() === "!=" || peek() === "==") { + const op = take() + const eq = lower(a) === lower(primary()) + return op === "==" ? eq : !eq + } + return a + } + function and() { + let v = compare() + while (peek() === "&&") { + take() + const r = compare() + v = Boolean(v) && Boolean(r) + } + return v + } + function or() { + let v = and() + while (peek() === "||") { + take() + const r = and() + v = Boolean(v) || Boolean(r) + } + return v + } + return Boolean(or()) + } + const comment = (body, type) => ({ github: { event_name: "issue_comment", event: { comment: { body, user: { type } } } } }) + + test("a bot's comment, the App's own included, never boots a runner", () => { + for (const body of ["@browserstack go", "go", "approve", "Reply go to run this"]) { + assert.equal(evaluate(condition, comment(body, "Bot")), false, `a bot's "${body}" would start a runner`) + } + }) + + test("a person's request, approval or mention still boots one", () => { + for (const body of ["@browserstack explain", "go", "Go", "approve"]) { + assert.equal(evaluate(condition, comment(body, "User")), true, `"${body}" from a person was filtered out`) + } + assert.equal(evaluate(condition, comment("thanks!", "User")), false) + }) + + test("other events are never filtered", () => { + assert.equal(evaluate(condition, { github: { event_name: "pull_request", event: {} } }), true) + }) + test("every other event is untouched, so pushes and schedules still run", () => { // The filter opens with the non-comment case for exactly this reason. const mentionCheck = job.indexOf("contains(github.event.comment.body, '@browserstack')")