test-companion: a plain go on a generate plan saves exactly the planned cases

test-companion/README.md

@@ -67,6 +67,16 @@ from the release you tested; a version without a checksum is refused.
 
 Before a job changes anything, Test Companion posts its plan and waits for
 someone with write access to reply `go`. Read the plan before you reply.
+Test Companion reacts 👀 to a request, or a `go`, once it picks it up.
+
+For `generate`, a plain `go` saves exactly the test cases in the plan to Test
+Management, in the project and folder the plan recorded: the plan's run keeps
+them (the `test-companion-cases` artifact, for 7 days) and nothing is generated
+again. A `@browserstack go` with changes generates the cases again with your
+changes as guidance, and says so. If the plan's cases can't be found, or the
+plan recorded no Test Management destination (set `tm_project` and `tm_folder`
+for generate in the pipeline file), nothing is saved and you are asked to
+generate a fresh plan.
 
 Without the GitHub App, Test Companion posts as `github-actions[bot]`, the
 account every workflow in your repository shares, and recognises its own plans

test-companion/action.yml

@@ -234,6 +234,18 @@ runs:
               path: ${{ steps.run.outputs.result-file }}
               if-no-files-found: ignore
 
+        # A generate plan's cases as data. Approving the plan saves exactly these, so nothing
+        # is generated again. Only a plan sets cases-dir.
+        - name: Keep the planned test cases for approval
+          if: always() && steps.run.outputs.cases-dir != ''
+          # v4.6.2, pinned to a commit: a tag can be repointed.
+          uses: actions/upload-artifact@ea165f8d65b6e75b540449e92b4886f43607fa02
+          with:
+              name: test-companion-cases
+              path: ${{ steps.run.outputs.cases-dir }}
+              if-no-files-found: ignore
+              retention-days: 7
+
         # Matters on a self-hosted runner, which outlives the job.
         - name: Take gh's credentials back off the runner
           if: always()

test-companion/src/access.mjs

@@ -8,6 +8,34 @@ export function mayAsk({ hasWriteAccess, askTeam, inTeam } = {}) {
 	return inTeam === true
 }
 
+/**
+ * The comment to mark with 👀 once a request is picked up, or undefined. Only a comment a
+ * person with access wrote to us: never a bot's, never a refused one, and a bare `go` only
+ * when there is a plan for it to approve.
+ */
+export function pickupReactionTarget({ eventName, comment, plan, allowed, approvalHasPlan } = {}) {
+	if (eventName !== "issue_comment" || !plan || plan.skip || plan.event !== "pr_comment") return undefined
+	if (allowed !== true) return undefined
+	const author = comment?.user
+	if (!author || author.type === "Bot" || /\[bot\]$/i.test(author.login ?? "")) return undefined
+	if (plan.approvalOnly && approvalHasPlan !== true) return undefined
+	const id = Number(comment?.id)
+	return Number.isSafeInteger(id) && id > 0 ? id : undefined
+}
+
+/**
+ * Add 👀 to a comment. Best-effort: `post(path, body)` is the API call; any failure,
+ * thrown or returned, is reported back and never stops the run. Returns true when added.
+ */
+export function addPickupReaction({ repo, commentId, post }) {
+	if (!repo || !commentId || typeof post !== "function") return false
+	try {
+		return post(`repos/${repo}/issues/comments/${commentId}/reactions`, { content: "eyes" }) === true
+	} catch {
+		return false
+	}
+}
+
 /** Whether this run can write to the configured tests repository. `canPush` undefined counts as no. */
 export function describeTestRepoAccess({ testsRepo, thisRepo, canPush } = {}) {
 	const target = String(testsRepo ?? "").trim()

test-companion/src/answer.mjs

@@ -5,24 +5,41 @@ 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 {
 			event = JSON.parse(t)
 		} 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,25 +68,72 @@ export function createAnswerCapture() {
 			pending = ""
 		},
 		answer: () => answer,
+		structuredError: () => structuredError,
+		plainError: () => plainError,
+		error: () => structuredError ?? plainError,
 		pending: () => pending.length,
 	}
 }
 
 /** 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)
+}

test-companion/src/app-auth.mjs

@@ -28,8 +28,51 @@ export function jwtClaims(appId, nowSeconds) {
  * and GitHub accepts an App's JWT only as `Authorization: Bearer`.
  * Returns { ok, status, data, reason }; `reason` is GitHub's message, never the JWT.
  */
-export async function appApi(path, { jwt, method = "GET", body, apiUrl, fetchImpl = fetch } = {}) {
+export async function appApi(
+	path,
+	{ jwt, method = "GET", body, apiUrl, fetchImpl = fetch, attempts = 3, sleep = defaultSleep, timeoutMs = APP_CALL_TIMEOUT_MS } = {},
+) {
+	let result
+	for (let attempt = 1; attempt <= attempts; attempt++) {
+		result = await appApiOnce(path, { jwt, method, body, apiUrl, fetchImpl, timeoutMs })
+		if (!isRetryable(result) || attempt === attempts) return { ...result, attempts: attempt }
+		await sleep(retryDelayMs(result, attempt))
+	}
+	return { ...result, attempts }
+}
+
+const defaultSleep = (ms) => new Promise((resolve) => setTimeout(resolve, ms))
+
+/** One App call gives up after this long; a timeout counts as a network error, so it is retried. */
+export const APP_CALL_TIMEOUT_MS = 10_000
+
+/** A network failure, a 5xx or a 429 may pass on a second try; 401/403/404/422 never will. */
+export function isRetryable(result) {
+	const status = Number(result?.status)
+	return status === 0 || status === 429 || (status >= 500 && status <= 599)
+}
+
+/** 1s, then 3s; a Retry-After from GitHub wins, capped at 10s. */
+export function retryDelayMs(result, attempt) {
+	const after = Number(result?.retryAfter)
+	if (Number.isFinite(after) && after > 0) return Math.min(after, 10) * 1000
+	return attempt === 1 ? 1000 : 3000
+}
+
+async function appApiOnce(path, { jwt, method, body, apiUrl, fetchImpl, timeoutMs }) {
 	const base = String(apiUrl || "https://api.github.com").replace(/\/+$/, "")
+	// Without a limit a stalled connection waits ~5 minutes, and each retry again. Covers the
+	// body too; aborting rejects fetch, which reads below as a network error and is retried.
+	const controller = new AbortController()
+	const timer = setTimeout(() => controller.abort(new Error(`no reply within ${timeoutMs / 1000}s`)), timeoutMs)
+	try {
+		return await appApiCall(base, path, { jwt, method, body, fetchImpl, signal: controller.signal })
+	} finally {
+		clearTimeout(timer)
+	}
+}
+
+async function appApiCall(base, path, { jwt, method, body, fetchImpl, signal }) {
 	let response
 	try {
 		response = await fetchImpl(`${base}/${path}`, {
@@ -41,13 +84,15 @@ export async function appApi(path, { jwt, method = "GET", body, apiUrl, fetchImp
 				...(body === undefined ? {} : { "Content-Type": "application/json" }),
 			},
 			body: body === undefined ? undefined : JSON.stringify(body),
+			signal,
 		})
 	} catch (e) {
 		return { ok: false, status: 0, data: undefined, reason: `could not reach GitHub: ${e?.message ?? e}` }
 	}
 	const data = await response.json().catch(() => undefined)
 	const reason = response.ok ? "" : `${data?.message || response.statusText || "no detail from GitHub"} (HTTP ${response.status})`
-	return { ok: response.ok, status: response.status, data, reason }
+	const retryAfter = response.headers?.get?.("retry-after") ?? undefined
+	return { ok: response.ok, status: response.status, data, reason, ...(retryAfter ? { retryAfter } : {}) }
 }
 
 /** What to log when looking up the App's installation on this repository fails. */