diff --git a/.github/workflows/release-smoke.yml b/.github/workflows/release-smoke.yml index 1656d4f16f..e0aae6d1a7 100644 --- a/.github/workflows/release-smoke.yml +++ b/.github/workflows/release-smoke.yml @@ -101,6 +101,13 @@ jobs: runs-on: ubuntu-latest timeout-minutes: 45 + # Fixed here rather than read back out of the harness, so the `always()` + # diagnostics steps below still know the container's name when the launch + # step is the thing that failed. Reading it back was why a failing smoke + # uploaded no Docker logs at all. + env: + SMOKE_CONTAINER_NAME: release-smoke-onboard + steps: - name: Checkout repository uses: actions/checkout@v7 @@ -126,26 +133,24 @@ jobs: - name: Launch Docker smoke harness run: | - metadata_file="$RUNNER_TEMP/release-smoke.env" HOST_PORT="${{ inputs.host_port }}" \ DATA_DIR="$RUNNER_TEMP/release-smoke-data" \ PAPERCLIPAI_VERSION="${{ inputs.paperclip_version }}" \ SMOKE_READY_TIMEOUT_SECONDS=420 \ SMOKE_DETACH=true \ - SMOKE_METADATA_FILE="$metadata_file" \ + SMOKE_METADATA_FILE="${{ runner.temp }}/release-smoke.env" \ + SMOKE_LOG_FILE="${{ runner.temp }}/docker-onboard-smoke.log" \ ./scripts/docker-onboard-smoke.sh set -a - source "$metadata_file" + source "${{ runner.temp }}/release-smoke.env" set +a { echo "SMOKE_BASE_URL=$SMOKE_BASE_URL" echo "SMOKE_ADMIN_EMAIL=$SMOKE_ADMIN_EMAIL" echo "SMOKE_ADMIN_PASSWORD=$SMOKE_ADMIN_PASSWORD" - echo "SMOKE_CONTAINER_NAME=$SMOKE_CONTAINER_NAME" echo "SMOKE_DATA_DIR=$SMOKE_DATA_DIR" echo "SMOKE_IMAGE_NAME=$SMOKE_IMAGE_NAME" echo "SMOKE_PAPERCLIPAI_VERSION=$SMOKE_PAPERCLIPAI_VERSION" - echo "SMOKE_METADATA_FILE=$metadata_file" } >> "$GITHUB_ENV" - name: Run release smoke Playwright suite @@ -159,9 +164,20 @@ jobs: - name: Capture Docker logs if: always() run: | - if [[ -n "${SMOKE_CONTAINER_NAME:-}" ]]; then - docker logs "$SMOKE_CONTAINER_NAME" >"$RUNNER_TEMP/docker-onboard-smoke.log" 2>&1 || true + log_file="${{ runner.temp }}/docker-onboard-smoke.log" + # A live container has the fuller story, so prefer it. When the + # harness already tore the container down it wrote this file on its + # way out, and that copy is kept rather than clobbered. + if docker inspect "$SMOKE_CONTAINER_NAME" >/dev/null 2>&1; then + docker logs "$SMOKE_CONTAINER_NAME" >"$log_file" 2>&1 || true fi + # Never leave the upload with nothing to say. An absent log reads as + # a missing artifact; a file saying the container was gone reads as + # the diagnosis it is. + if [[ ! -s "$log_file" ]]; then + echo "No Docker logs captured: container '$SMOKE_CONTAINER_NAME' left no log dump and is no longer present." >"$log_file" + fi + echo "Captured $(wc -l <"$log_file") log lines to $log_file" - name: Upload diagnostics if: always() @@ -170,14 +186,15 @@ jobs: name: ${{ inputs.artifact_name }} path: | ${{ runner.temp }}/docker-onboard-smoke.log - ${{ env.SMOKE_METADATA_FILE }} + ${{ runner.temp }}/release-smoke.env tests/release-smoke/playwright-report/ tests/release-smoke/test-results/ + # The capture step above guarantees the log file, so an empty upload + # means the diagnostics wiring itself broke — which is worth failing + # over rather than burying in a warning nobody reads. + if-no-files-found: error retention-days: 14 - name: Stop Docker smoke container if: always() - run: | - if [[ -n "${SMOKE_CONTAINER_NAME:-}" ]]; then - docker rm -f "$SMOKE_CONTAINER_NAME" >/dev/null 2>&1 || true - fi + run: docker rm -f "$SMOKE_CONTAINER_NAME" >/dev/null 2>&1 || true diff --git a/doc/DOCKER.md b/doc/DOCKER.md index b29fc2b147..edac854ac2 100644 --- a/doc/DOCKER.md +++ b/doc/DOCKER.md @@ -259,6 +259,8 @@ Notes: - In authenticated mode, the smoke script defaults `SMOKE_AUTO_BOOTSTRAP=true` and drives the real bootstrap path automatically: it signs up a real user, runs `paperclipai auth bootstrap-ceo` inside the container to mint a real bootstrap invite, accepts that invite over HTTP, and verifies board session access. - Run the script in the foreground to watch the onboarding flow; stop with `Ctrl+C` after validation. - Set `SMOKE_DETACH=true` to leave the container running for automation and optionally write shell-ready metadata to `SMOKE_METADATA_FILE`. +- Set `SMOKE_CONTAINER_NAME` to fix the container's name up front. Automation that has to collect diagnostics when the script *fails* needs a name it already knows, rather than one it can only read back out of a successful run. Defaults to the image name. +- The container's logs are dumped to `SMOKE_LOG_FILE` (default `$TMPDIR/.log`) before the script tears the container down, so a run that never became ready still leaves its logs behind. - The image definition is in `docker/Dockerfile.onboard-smoke`. ## General Notes diff --git a/package.json b/package.json index b24f168ca6..2dd77a1675 100644 --- a/package.json +++ b/package.json @@ -54,7 +54,7 @@ "smoke:mcp-fixtures": "node scripts/smoke/mcp-fixture-harness.mjs", "smoke:pipelines-tutorial": "./scripts/smoke/pipelines-tutorial-smoke.sh", "smoke:terminal-bench-loop-skill": "node scripts/smoke/terminal-bench-loop-skill-smoke.mjs", - "test:release-registry": "node --test scripts/verify-release-registry-state.test.mjs scripts/release-package-map.test.mjs scripts/check-release-package-bootstrap.test.mjs scripts/check-no-git-push.test.mjs scripts/release-lib.test.mjs scripts/release-registry-versions.test.mjs scripts/link-plugin-dev-sdk.test.js scripts/acpx-patch-packaging.test.mjs scripts/service-onboard-smoke.test.mjs", + "test:release-registry": "node --test scripts/verify-release-registry-state.test.mjs scripts/release-package-map.test.mjs scripts/check-release-package-bootstrap.test.mjs scripts/check-no-git-push.test.mjs scripts/release-lib.test.mjs scripts/release-registry-versions.test.mjs scripts/link-plugin-dev-sdk.test.js scripts/acpx-patch-packaging.test.mjs scripts/service-onboard-smoke.test.mjs scripts/docker-onboard-smoke.test.mjs", "storybook-visual:baseline": "node scripts/storybook-visual-baseline.mjs", "test:storybook-visual": "node scripts/storybook-visual-baseline.mjs download && node scripts/storybook-visual-baseline.mjs verify && pnpm build-storybook && npx playwright test --config tests/storybook-visual/playwright.config.ts", "test:storybook-visual:update": "node scripts/storybook-visual-baseline.mjs download && pnpm build-storybook && npx playwright test --config tests/storybook-visual/playwright.config.ts --update-snapshots && node scripts/storybook-visual-baseline.mjs pack", diff --git a/scripts/docker-onboard-smoke.sh b/scripts/docker-onboard-smoke.sh index 548931b557..18deed3d90 100755 --- a/scripts/docker-onboard-smoke.sh +++ b/scripts/docker-onboard-smoke.sh @@ -21,7 +21,15 @@ SMOKE_READY_TIMEOUT_SECONDS="${SMOKE_READY_TIMEOUT_SECONDS:-90}" SMOKE_ADMIN_NAME="${SMOKE_ADMIN_NAME:-Smoke Admin}" SMOKE_ADMIN_EMAIL="${SMOKE_ADMIN_EMAIL:-smoke-admin@paperclip.local}" SMOKE_ADMIN_PASSWORD="${SMOKE_ADMIN_PASSWORD:-paperclip-smoke-password}" -CONTAINER_NAME="${IMAGE_NAME//[^a-zA-Z0-9_.-]/-}" +# Overridable so a caller can fix the name before this script runs. CI needs +# that: a name it only learns from this script's output is a name it does not +# have when this script fails, which is precisely when its diagnostics steps +# need one. +CONTAINER_NAME="${SMOKE_CONTAINER_NAME:-$IMAGE_NAME}" +CONTAINER_NAME="${CONTAINER_NAME//[^a-zA-Z0-9_.-]/-}" +# Where the container's logs are written before it is torn down. See +# `dump_container_logs`. +SMOKE_LOG_FILE="${SMOKE_LOG_FILE:-${TMPDIR:-/tmp}/${CONTAINER_NAME}.log}" LOG_PID="" COOKIE_JAR="" TMP_DIR="" @@ -29,12 +37,46 @@ PRESERVE_CONTAINER_ON_EXIT="false" mkdir -p "$DATA_DIR" +# Start from an empty dump. `dump_container_logs` only writes when there is a +# container to read, so a run that fails before one exists — a failed build, a +# port already bound — would otherwise leave the previous run's file in place, +# and that file would be read as this run's diagnostics. Truncated rather than +# removed, so the path is present and writable from here on. +if [[ -n "$SMOKE_LOG_FILE" ]]; then + mkdir -p "$(dirname "$SMOKE_LOG_FILE")" >/dev/null 2>&1 || true + : >"$SMOKE_LOG_FILE" 2>/dev/null || true +fi + +# Copy the container's logs out while there is still a container to read them +# from. +# +# This runs on every failure path — the image failing to serve, health never +# coming up, bootstrap rejecting the admin — which is exactly when the logs are +# the only account of what went wrong, and exactly when they used to be +# destroyed unread: `docker run` passed `--rm`, so the container and its logs +# went away with the stop below (and, for a container that crashed on its own, +# the moment its process exited). `--rm` is gone for that reason; removal is +# this script's job now, and it happens after the dump. +dump_container_logs() { + if [[ -z "$SMOKE_LOG_FILE" ]]; then + return 0 + fi + if ! docker inspect "$CONTAINER_NAME" >/dev/null 2>&1; then + return 0 + fi + mkdir -p "$(dirname "$SMOKE_LOG_FILE")" >/dev/null 2>&1 || return 0 + docker logs "$CONTAINER_NAME" >"$SMOKE_LOG_FILE" 2>&1 || true +} + cleanup() { if [[ -n "$LOG_PID" ]]; then kill "$LOG_PID" >/dev/null 2>&1 || true fi + # Before the teardown below, never after it. + dump_container_logs if [[ "$PRESERVE_CONTAINER_ON_EXIT" != "true" ]]; then docker stop "$CONTAINER_NAME" >/dev/null 2>&1 || true + docker rm -f "$CONTAINER_NAME" >/dev/null 2>&1 || true fi if [[ -n "$TMP_DIR" && -d "$TMP_DIR" ]]; then rm -rf "$TMP_DIR" @@ -85,6 +127,7 @@ write_metadata_file() { printf 'SMOKE_ADMIN_EMAIL=%q\n' "$SMOKE_ADMIN_EMAIL" printf 'SMOKE_ADMIN_PASSWORD=%q\n' "$SMOKE_ADMIN_PASSWORD" printf 'SMOKE_CONTAINER_NAME=%q\n' "$CONTAINER_NAME" + printf 'SMOKE_LOG_FILE=%q\n' "$SMOKE_LOG_FILE" printf 'SMOKE_DATA_DIR=%q\n' "$DATA_DIR" printf 'SMOKE_IMAGE_NAME=%q\n' "$IMAGE_NAME" printf 'SMOKE_PAPERCLIPAI_VERSION=%q\n' "$PAPERCLIPAI_VERSION" @@ -260,6 +303,8 @@ echo " Public URL: $PAPERCLIP_PUBLIC_URL" echo " Smoke auto-bootstrap: $SMOKE_AUTO_BOOTSTRAP" echo " Detached mode: $SMOKE_DETACH" echo " Data dir: $DATA_DIR" +echo " Container name: $CONTAINER_NAME" +echo " Container log dump: $SMOKE_LOG_FILE" echo " Deployment: $PAPERCLIP_DEPLOYMENT_MODE/$PAPERCLIP_DEPLOYMENT_EXPOSURE" if [[ "$SMOKE_DETACH" != "true" ]]; then echo " Live output: onboard banner and server logs stream in this terminal (Ctrl+C to stop)" @@ -267,7 +312,11 @@ fi docker rm -f "$CONTAINER_NAME" >/dev/null 2>&1 || true -docker run -d --rm \ +# No `--rm`. A container that removes itself takes its logs with it the instant +# it exits, which is the one moment they are worth reading; the cleanup above +# removes it instead, after dumping them. The `docker rm -f` just above covers +# a container left behind by a previous run. +docker run -d \ --name "$CONTAINER_NAME" \ -p "$HOST_PORT:3100" \ -e HOST=0.0.0.0 \ diff --git a/scripts/docker-onboard-smoke.test.mjs b/scripts/docker-onboard-smoke.test.mjs new file mode 100644 index 0000000000..6354a3e8e3 --- /dev/null +++ b/scripts/docker-onboard-smoke.test.mjs @@ -0,0 +1,106 @@ +import assert from "node:assert/strict"; +import { execFileSync } from "node:child_process"; +import { accessSync, constants, readFileSync } from "node:fs"; +import { join } from "node:path"; +import test from "node:test"; + +// Pins the diagnostics wiring of the Docker leg of the release smoke. +// +// The smoke itself only runs post-merge, against a published artifact, so its +// own failures are the only signal it ever sends — and for a long time that +// signal arrived with no container logs attached: the workflow learned the +// container's name from the harness's output, which a failing harness never +// produced, and the harness ran the container with `--rm` so stopping it +// deleted the logs anyway. These assertions keep both halves fixed. + +const repoRoot = new URL("..", import.meta.url).pathname.replace(/\/$/, ""); +const scriptPath = join(repoRoot, "scripts", "docker-onboard-smoke.sh"); +const script = readFileSync(scriptPath, "utf8"); +const workflow = readFileSync( + join(repoRoot, ".github", "workflows", "release-smoke.yml"), + "utf8", +); +const dockerJob = workflow.split(/^ smoke:$/m)[1] ?? ""; + +test("smoke script is executable and parses", () => { + accessSync(scriptPath, constants.X_OK); + execFileSync("bash", ["-n", scriptPath]); +}); + +test("container name can be fixed by the caller", () => { + // A name the caller chose is a name it still has when this script fails. + assert.match(script, /CONTAINER_NAME="\$\{SMOKE_CONTAINER_NAME:-\$IMAGE_NAME\}"/); + // And it is still sanitized into something Docker will accept. + assert.match(script, /CONTAINER_NAME="\$\{CONTAINER_NAME\/\/\[\^a-zA-Z0-9_\.-\]\/-\}"/); +}); + +test("the container does not remove itself", () => { + // `--rm` deletes the container the instant its process exits, so a crash + // takes the logs with it before any cleanup can read them. + assert.match(script, /^docker run -d \\$/m); + assert.doesNotMatch(script, /docker run [^\n]*--rm/); +}); + +test("cleanup dumps the container logs before it tears the container down", () => { + const cleanup = script.match(/^cleanup\(\) \{$[\s\S]*?^\}$/m)?.[0]; + assert.ok(cleanup, "cleanup() must exist"); + const dumpAt = cleanup.indexOf("dump_container_logs"); + const stopAt = cleanup.indexOf("docker stop"); + const removeAt = cleanup.indexOf("docker rm"); + assert.ok(dumpAt !== -1, "cleanup() must dump the container logs"); + assert.ok(stopAt !== -1, "cleanup() must still stop the container"); + assert.ok( + removeAt !== -1, + "cleanup() must remove the container now that it no longer removes itself", + ); + assert.ok( + dumpAt < stopAt && dumpAt < removeAt, + "cleanup() must dump the logs before the teardown, or the teardown deletes them first", + ); +}); + +test("the log dump has a destination, and callers are told where it is", () => { + assert.match(script, /SMOKE_LOG_FILE="\$\{SMOKE_LOG_FILE:-/); + assert.match(script, /docker logs "\$CONTAINER_NAME" >"\$SMOKE_LOG_FILE"/); + assert.match(script, /printf 'SMOKE_LOG_FILE=%q\\n' "\$SMOKE_LOG_FILE"/); +}); + +test("the log dump starts empty on every run", () => { + // A caller that reuses one path — the default does, for a fixed container + // name — must not be handed the previous run's logs as this run's evidence + // when this run fails before a container exists. + const truncateAt = script.search(/^\s*: >"\$SMOKE_LOG_FILE"/m); + const dumpAt = script.indexOf("dump_container_logs() {"); + assert.ok(truncateAt !== -1, "the script must truncate SMOKE_LOG_FILE at startup"); + assert.ok( + truncateAt < dumpAt, + "the truncation must happen before anything can write the dump", + ); +}); + +test("workflow fixes the container name before the harness runs", () => { + assert.match(dockerJob, /^ env:$/m); + assert.match(dockerJob, /SMOKE_CONTAINER_NAME: release-smoke-onboard/); + // Reading the name back out of the harness is the defect: a step that only + // learns it on success cannot use it on failure. + assert.doesNotMatch(dockerJob, /echo "SMOKE_CONTAINER_NAME=/); + assert.match(dockerJob, /SMOKE_LOG_FILE="\$\{\{ runner\.temp \}\}\/docker-onboard-smoke\.log"/); +}); + +test("workflow captures and uploads the logs unconditionally", () => { + const capture = dockerJob.split("- name: Capture Docker logs")[1] ?? ""; + assert.ok(capture, "the Capture Docker logs step must exist"); + assert.match(capture.split("- name:")[0], /if: always\(\)/); + // No guard that a failing launch would leave false. + assert.doesNotMatch( + capture.split("- name:")[0], + /\[\[ -n "\$\{SMOKE_CONTAINER_NAME:-\}" \]\]/, + ); + + const upload = dockerJob.split("- name: Upload diagnostics")[1] ?? ""; + assert.ok(upload, "the Upload diagnostics step must exist"); + assert.match(upload, /docker-onboard-smoke\.log/); + assert.match(upload, /if-no-files-found: error/); + // The metadata path is a literal, not a variable a failed launch never set. + assert.doesNotMatch(upload, /\$\{\{ env\.SMOKE_METADATA_FILE \}\}/); +}); diff --git a/tests/release-smoke/docker-auth-onboarding.spec.ts b/tests/release-smoke/docker-auth-onboarding.spec.ts index 650c24a2ea..5f443abb16 100644 --- a/tests/release-smoke/docker-auth-onboarding.spec.ts +++ b/tests/release-smoke/docker-auth-onboarding.spec.ts @@ -10,7 +10,10 @@ const ADMIN_PASSWORD = "paperclip-smoke-password"; const COMPANY_NAME = `Release-Smoke-${Date.now()}`; -const AGENT_NAME = "CEO"; +const AGENT_NAME = "Release Smoke Lead"; +// The arc asks for a name, not a role, so every onboarding hire is filed under +// the neutral role (DEFAULT_AGENT_ROLE in ui/src/lib/onboarding-agent-role.ts). +const AGENT_ROLE = "general"; // Seeded by the wizard's launch step (DEFAULT_TASK_TITLE in // ui/src/components/OnboardingWizard.tsx). const FIRST_TASK_TITLE = "Paperclip onboarding"; @@ -26,17 +29,47 @@ async function signIn(page: Page) { await expect(page).not.toHaveURL(/\/auth/, { timeout: 20_000 }); } +async function getJson(page: Page, url: string): Promise { + const response = await page.request.get(url); + expect(response.ok()).toBe(true); + return (await response.json()) as T; +} + +// ONBOARDING_STORAGE_KEY in ui/src/components/OnboardingWizard.tsx. +const ONBOARDING_DRAFT_STORAGE_KEY = "paperclip-onboarding-state"; + +/** + * Open the wizard on its first step and hand back the organization-name field. + * + * `/onboarding` resolves to `{ initialStep: 1 }` on a self-hosted instance + * (`resolveRouteOnboardingOptions`) and the route keeps the wizard open, so + * this lands on "name your organization" whether or not the instance already + * holds a company. Navigating explicitly is what keeps the spec re-runnable: + * the release-smoke config retries once in CI, and by the second attempt the + * instance is no longer company-less, so sign-in lands on a dashboard instead. + * + * The saved draft is dropped first. Sign-in on an instance that already holds + * an agentless company redirects into *that* company's onboarding, which + * persists its id into the draft; the restored draft then makes step 1 skip + * creating a company and hire into the old one instead. That is an artifact of + * re-running against a re-used instance, not behaviour this spec is asserting, + * and a fresh release-smoke container never has it. + * + * The field is located by role. Step 1 has no id and its `