From 8a13d76de26e9d592096fcd989c6fd8d468ec75d Mon Sep 17 00:00:00 2001 From: FlyBot Dev-3 Date: Mon, 2 Nov 2026 18:24:48 +0000 Subject: [PATCH] fix(live-e2e): one reading of DEEPSEEK_API_KEY for the preflight, the bench and the guard (#2214) The CI preflight published key=true for any non-empty string, while the bench starter and the run guard refused the mock's key and anything under 16 characters. So a short or mock key skipped nothing: the bench started and failed the night for exactly the reason the #794 skip exists to avoid. One module (dev/live-e2e/key.mjs) now returns the verdict and the words for it, and all three readers use it. A key that is present but unusable is skipped with its own warning and its own summary, so the skip never reads as a pass and the reason is visible without reading a red log. A key long enough but not real is still not detected here; only the endpoint can judge that. --- .github/scripts/live-e2e-preflight.mjs | 34 ++++++++++++++----- .github/scripts/live-e2e-preflight.test.mjs | 17 ++++++++-- .../2214-live-e2e-key-reading.changed.md | 1 + dev/live-e2e/README.md | 2 +- dev/live-e2e/bench.mjs | 11 +++--- dev/live-e2e/key.mjs | 23 +++++++++++++ dev/live-e2e/report.mjs | 7 ++-- dev/live-e2e/test/key-reading.test.mjs | 34 +++++++++++++++++++ docs/subsystems/live-e2e.md | 4 +++ 9 files changed, 115 insertions(+), 18 deletions(-) create mode 100644 changelog.d/2214-live-e2e-key-reading.changed.md create mode 100644 dev/live-e2e/key.mjs create mode 100644 dev/live-e2e/test/key-reading.test.mjs diff --git a/.github/scripts/live-e2e-preflight.mjs b/.github/scripts/live-e2e-preflight.mjs index f9b484e1a..ce9c45471 100644 --- a/.github/scripts/live-e2e-preflight.mjs +++ b/.github/scripts/live-e2e-preflight.mjs @@ -1,14 +1,19 @@ #!/usr/bin/env node // The live E2E preflight (#794). The suite cannot run without DEEPSEEK_API_KEY, and a run that is -// red every night for a reason nobody can act on teaches the team to ignore red. So a missing key -// is a warning, not a failure: this step publishes `key=false`, the workflow skips every step that -// needs the key, and the summary says the run measured nothing. A failure inside the suite still -// fails the run, so the skip is never a pass. +// red every night for a reason nobody can act on teaches the team to ignore red. So a key the suite +// cannot use is a warning, not a failure: this step publishes `key=false`, the workflow skips every +// step that needs the key, and the summary says the run measured nothing. A failure inside the +// suite still fails the run, so the skip is never a pass. +// +// "Cannot use" is the suite's own reading (`dev/live-e2e/key.mjs`), not this step's: the bench and +// the run guard refuse a key under 16 characters or the mock's, and a preflight that accepted them +// let those keys through to a red bench instead of the skip. // // `.github/workflows/live-e2e.yml` runs it as `node .github/scripts/live-e2e-preflight.mjs`, with // the secret mapped into DEEPSEEK_API_KEY step-scoped and nothing else in the environment. import { appendFileSync } from 'node:fs' import { fileURLToPath } from 'node:url' +import { keyReading } from '../../dev/live-e2e/key.mjs' export const WARNING = '::warning title=Live E2E skipped::DEEPSEEK_API_KEY is not configured as a repository secret; the four real-model checks and the evidence bundle are skipped, so this run measured nothing' @@ -20,15 +25,26 @@ export const SUMMARY = [ '**This run is green because there was nothing to run, not because the suite passed.** Add the secret in Settings → Secrets and variables → Actions to measure the suite again (#794).', ] +export const UNUSABLE_WARNING = '::warning title=Live E2E skipped::DEEPSEEK_API_KEY is set but is not a usable key (the mock\'s key, or shorter than 16 characters); the four real-model checks and the evidence bundle are skipped, so this run measured nothing' + +export const UNUSABLE_SUMMARY = [ + '### Live E2E skipped: the repository secret is not a usable key', + '', + '`DEEPSEEK_API_KEY` is set, but the suite refuses it: the mock\'s key or fewer than 16 characters never reaches the real endpoint, so no step of the suite ran and no evidence bundle was produced.', + '', + '**This run is green because there was nothing to run, not because the suite passed.** Replace the secret in Settings → Secrets and variables → Actions with a real key to measure the suite again.', +] + // A secret that is not configured reaches the step as an empty string, not as a missing variable, // so both have to mean the same thing here. export function plan(key) { - const present = typeof key === 'string' && key.length > 0 + const { usable, reason } = keyReading(key) + const missing = reason === 'missing' return { - present, - output: `key=${present}`, - warning: present ? null : WARNING, - summary: present ? null : SUMMARY.join('\n'), + present: usable, + output: `key=${usable}`, + warning: usable ? null : (missing ? WARNING : UNUSABLE_WARNING), + summary: usable ? null : (missing ? SUMMARY : UNUSABLE_SUMMARY).join('\n'), } } diff --git a/.github/scripts/live-e2e-preflight.test.mjs b/.github/scripts/live-e2e-preflight.test.mjs index ba25034a8..93e69dbc7 100644 --- a/.github/scripts/live-e2e-preflight.test.mjs +++ b/.github/scripts/live-e2e-preflight.test.mjs @@ -11,7 +11,7 @@ import { join } from 'node:path' import { SUMMARY, WARNING, plan } from './live-e2e-preflight.mjs' test('#794: a configured key runs the suite and says nothing', () => { - const step = plan('sk-live-e2e') + const step = plan('sk-live-e2e-0123456789abcdef') assert.equal(step.output, 'key=true') assert.equal(step.warning, null) assert.equal(step.summary, null) @@ -52,10 +52,23 @@ test('#794: with a key the step writes key=true and adds no warning or summary', writeFileSync(summary, '') const result = spawnSync(process.execPath, [join(import.meta.dirname, 'live-e2e-preflight.mjs')], { encoding: 'utf8', - env: { PATH: process.env.PATH, DEEPSEEK_API_KEY: 'sk-live-e2e', GITHUB_OUTPUT: output, GITHUB_STEP_SUMMARY: summary }, + env: { PATH: process.env.PATH, DEEPSEEK_API_KEY: 'sk-live-e2e-0123456789abcdef', GITHUB_OUTPUT: output, GITHUB_STEP_SUMMARY: summary }, }) assert.equal(result.status, 0) assert.equal(result.stdout, '') assert.equal(readFileSync(output, 'utf8').trim(), 'key=true') assert.equal(readFileSync(summary, 'utf8'), '') }) + +test('a key that is present but unusable skips like a missing one, and says why', () => { + assert.equal(plan(' ').warning, WARNING, 'a blank secret is the same thing as no secret') + for (const key of ['mock-key', 'short', 'a'.repeat(15)]) { + const step = plan(key) + assert.equal(step.output, 'key=false', `the preflight lets ${JSON.stringify(key)} through`) + assert.match(step.warning, /^::warning /) + assert.match(step.warning, /not a usable key/) + assert.notEqual(step.warning, WARNING, 'a key someone set is a different warning from no key at all') + assert.match(step.summary, /not a usable key/) + assert.match(step.summary, /nothing to run, not because the suite passed/) + } +}) diff --git a/changelog.d/2214-live-e2e-key-reading.changed.md b/changelog.d/2214-live-e2e-key-reading.changed.md new file mode 100644 index 000000000..d09b8756b --- /dev/null +++ b/changelog.d/2214-live-e2e-key-reading.changed.md @@ -0,0 +1 @@ +A `Live E2E` run no longer publishes a key the suite itself refuses: the preflight that decides whether any step runs, the bench starter and the run guard now read `DEEPSEEK_API_KEY` through one module, so the mock's key or one under 16 characters is skipped with its own warning — saying plainly that nothing ran — instead of reaching a bench that refuses it and failing the night for a reason the skip exists to avoid. A key long enough but not real is not detected here; only the endpoint can judge that ([#2214](https://github.com/ThinkFlowLab/FlyBot/issues/2214)). diff --git a/dev/live-e2e/README.md b/dev/live-e2e/README.md index e55512646..653e380d3 100644 --- a/dev/live-e2e/README.md +++ b/dev/live-e2e/README.md @@ -57,7 +57,7 @@ a real `DEEPSEEK_API_KEY` in the environment. Chrome comes from `CHROME_BIN`, el `google-chrome`/`chromium`(-`browser`) on PATH, else playwright's own download. ```sh -export DEEPSEEK_API_KEY=... # real key; the runner fails closed without it +export DEEPSEEK_API_KEY=... # real key (16+ chars, never the mock's); fails closed export LIVE_E2E_HOME="$HOME/.cache/flybot-live-e2e/live-e2e" # outside the checkout node dev/live-e2e/bench.mjs live-e2e 3196 reset node dev/live-e2e/run.mjs "$LIVE_E2E_HOME" 3196 --model deepseek-official/deepseek-flash diff --git a/dev/live-e2e/bench.mjs b/dev/live-e2e/bench.mjs index 9e75dc159..507ccb3ff 100644 --- a/dev/live-e2e/bench.mjs +++ b/dev/live-e2e/bench.mjs @@ -8,14 +8,16 @@ // Data lives outside this checkout — `$LIVE_E2E_HOME`, else ~/.cache/flybot-live-e2e/ (never // under ~/.dsh; `reset` wipes the Bot team and Sessions before starting). A home inside the // checkout is refused: that workspace would hold the grader (#398). Environment: DEEPSEEK_API_KEY -// (required; fails loudly when missing or the mock's), DEEPSEEK_BASE_URL (optional; defaults to the -// official endpoint — point it at dev/llm-tap.mjs to record per-call usage), DSH_HARNESS (a -// deepseek-harness checkout with its dependencies installed). +// (required; fails loudly when the suite's own reading refuses it — see key.mjs), +// DEEPSEEK_BASE_URL (optional; defaults to the official endpoint — point it at +// dev/llm-tap.mjs to record per-call usage), DSH_HARNESS (a deepseek-harness checkout with +// its dependencies installed). import { spawn, spawnSync } from 'node:child_process' import { existsSync, mkdirSync, openSync, readdirSync, readFileSync, rmSync, writeFileSync } from 'node:fs' import { dirname, join } from 'node:path' import { fileURLToPath } from 'node:url' import { benchHome } from './integrity.mjs' +import { keyProblem, keyReading } from './key.mjs' const ROOT = dirname(dirname(dirname(fileURLToPath(import.meta.url)))) const [NAME, PORT_TEXT, ACTION = 'start'] = process.argv.slice(2) @@ -29,7 +31,8 @@ const { home: HOME_DIR, problem: homeProblem } = benchHome({ name: NAME, root: R if (homeProblem) die(`${homeProblem}\n(the bench home must be outside ${ROOT}; export LIVE_E2E_HOME to choose it)`) const apiKey = process.env.DEEPSEEK_API_KEY ?? '' -if (!apiKey || apiKey === 'mock-key' || apiKey.length < 16) die('DEEPSEEK_API_KEY is missing, too short, or the mock\'s; a real key is required (never printed)') +const key = keyReading(apiKey) +if (!key.usable) die(`DEEPSEEK_API_KEY ${keyProblem(key.reason)}; a real key is required (never printed)`) const harness = process.env.DSH_HARNESS ?? join(ROOT, 'deepseek-harness') if (!existsSync(join(harness, 'package.json'))) die(`no deepseek-harness checkout at ${harness}; set DSH_HARNESS (see CONTRIBUTING.md)`) diff --git a/dev/live-e2e/key.mjs b/dev/live-e2e/key.mjs new file mode 100644 index 000000000..ed2cac325 --- /dev/null +++ b/dev/live-e2e/key.mjs @@ -0,0 +1,23 @@ +// One reading of DEEPSEEK_API_KEY for the three places that judge it: the CI preflight that decides +// whether any step runs (#794), the bench starter, and the run guard. They disagreed — the preflight +// took any non-empty string, the other two refused a key under 16 characters or the mock's — so a +// short or mock key published `key=true`, started the bench and failed the night for exactly the +// reason the skip exists to avoid. A key present but unusable is skipped like a missing one, with +// its own warning, so the skip never reads as a pass and nobody has to read a red log to find it. +export const MIN_LENGTH = 16 +export const MOCK_KEY = 'mock-key' + +export function keyReading(value) { + const key = typeof value === 'string' ? value.trim() : '' + if (key === '') return { usable: false, reason: 'missing' } + if (key === MOCK_KEY) return { usable: false, reason: 'mock' } + if (key.length < MIN_LENGTH) return { usable: false, reason: 'short' } + return { usable: true, reason: null } +} + +// The verdict in words, shared so the three readers cannot drift apart in what they say. +export function keyProblem(reason) { + if (reason === 'missing') return 'is missing' + if (reason === 'mock') return "is the mock's key" + return `is shorter than ${MIN_LENGTH} characters` +} diff --git a/dev/live-e2e/report.mjs b/dev/live-e2e/report.mjs index ea78a10c0..ea929c91c 100644 --- a/dev/live-e2e/report.mjs +++ b/dev/live-e2e/report.mjs @@ -1,5 +1,6 @@ import { join } from "node:path" import { canonicalPath, isInside } from "./integrity.mjs" +import { keyProblem, keyReading } from "./key.mjs" // The outcome contract and the report assembly for the #204 live runner. Pure code, // covered keyless by test/report.test.mjs. @@ -30,10 +31,12 @@ export function budgetsFrom(env) { // Fail-closed checks on the run's inputs. A loopback base URL is the mock bench (or the // usage tap, which names the real upstream in LIVE_E2E_TAP_UPSTREAM); anything else -// without a key, or with the mock's key, is refused rather than run. +// without a usable key (dev/live-e2e/key.mjs, the reading the preflight and the bench start share) +// is refused rather than run. export function guardRun({ apiKey, baseUrl, tapUpstream, home, userHome, repoRoot, evidence, cwd }) { const problems = [] - if (!apiKey || apiKey === 'mock-key' || apiKey.length < 16) problems.push('DEEPSEEK_API_KEY is missing, too short, or the mock key') + const key = keyReading(apiKey) + if (!key.usable) problems.push(`DEEPSEEK_API_KEY ${keyProblem(key.reason)}`) let host = 'api.deepseek.com' let origin = 'default official endpoint' if (baseUrl) { diff --git a/dev/live-e2e/test/key-reading.test.mjs b/dev/live-e2e/test/key-reading.test.mjs new file mode 100644 index 000000000..3ba588125 --- /dev/null +++ b/dev/live-e2e/test/key-reading.test.mjs @@ -0,0 +1,34 @@ +// The suite judges DEEPSEEK_API_KEY in three places: the CI preflight that decides whether any +// step runs, the bench starter, and the run guard that fails the run closed. Two of them refused +// a key the third one accepted, so a short or mock key published `key=true`, ran the bench and +// failed the night for a reason the skip exists to avoid (#794). One reading, three readers. +import assert from 'node:assert/strict' +import { readFileSync } from 'node:fs' +import { test } from 'node:test' +import { plan } from '../../../.github/scripts/live-e2e-preflight.mjs' +import { guardRun } from '../report.mjs' + +const CASES = [ + ['', 'a missing key'], + [undefined, 'an unset key'], + [' ', 'a blank key'], + ['mock-key', "the mock's key"], + ['short', 'a short key'], + ['a'.repeat(15), 'a key one character under the floor'], + ['a'.repeat(16), 'a key exactly at the floor'], + ['sk-' + 'a'.repeat(32), 'a real-looking key'], +] + +const keyProblems = (apiKey) => guardRun({ apiKey, baseUrl: undefined }).problems.filter((problem) => problem.startsWith('DEEPSEEK_API_KEY')) + +test('the preflight and the run guard accept exactly the same keys', () => { + for (const [key, what] of CASES) { + assert.equal(plan(key).present, keyProblems(key).length === 0, `the preflight and the run guard disagree about ${what}`) + } +}) + +test('the bench starter reads the key through the same module, with no threshold of its own', () => { + const bench = readFileSync(new URL('../bench.mjs', import.meta.url), 'utf8') + assert.doesNotMatch(bench, /\blength\s*