From dbf052577d073bdac511d796e763c0bbd0bd9bab Mon Sep 17 00:00:00 2001 From: Devin Foley Date: Fri, 28 Aug 2026 07:21:08 -0700 Subject: [PATCH] Follow the current onboarding arc in the release smoke (#12423) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work > - A release is gated by the release smoke: it installs the published `paperclipai` artifact into a Docker container and drives the sign-in → onboarding → first-agent path with Playwright > - That suite runs only from the release pipeline, never on a pull request, so it sees the UI only after the UI has already changed > - The onboarding wizard was rebuilt into the agent arc. The "Name your organization" step, the "Start Onboarding" launcher, and the agent role picker are all gone > - The spec still waited for those, so it failed on its first assertion and blocked every nightly and beta release > - The failure was also hard to read. The workflow uploaded no container logs, because it learned the container's name only after the harness succeeded, and the harness ran the container with `--rm` and deleted it before anything read it > - This pull request rewrites the spec to follow the current arc, and repairs the log capture at both ends > - The benefit is that nightly and beta releases are unblocked, and the next failure arrives with the logs attached ## Linked Issues or Issue Description No existing issue. Describing it inline, following `.github/ISSUE_TEMPLATE/bug_report.yml`. Refs #12274 (removed the company-naming step from the wizard). Refs #12135 (the previous alignment of this spec, before #12274). Refs #12316 (open; also edits `scripts/docker-onboard-smoke.sh`, in the bootstrap helpers rather than the container lifecycle, so the two changes do not overlap. Whichever lands second should rebase and re-run). **What happened?** The release smoke fails. `tests/release-smoke/docker-auth-onboarding.spec.ts` never gets past its first wait: ``` ✘ tests/release-smoke/docker-auth-onboarding.spec.ts:43:3 › Docker authenticated onboarding smoke › logs in, completes onboarding, and hires the lead agent Error: expect(locator).toBeVisible() failed — element(s) not found (timeout 20000ms) > 33 | await expect(wizardHeading.or(startButton)).toBeVisible({ timeout: 20_000 }); ``` The spec waits for an `h3` reading "Name your organization" or a "Start Onboarding" button. Neither exists. #12274 removed the company-naming step; the string now survives only in a code comment and in `ui/src/components/OnboardingWizard.step.test.tsx`, which asserts it is *absent*. The steps after the first wait are stale too: the CTA on step 1 is "Continue" and not "Next", the organization input's placeholder changed, and the agent step's `#onboarding-agent-role` picker is gone, so every onboarding hire is filed under the neutral `general` role. The suite runs only from the release pipeline, so nothing on a pull request saw the drift. Both `smoke_nightly` and `smoke_beta` call the same reusable workflow, so every nightly and every beta was blocked. The failure also arrived without diagnostics. The job's "Capture Docker logs" step is `if: always()`, but it is guarded on `SMOKE_CONTAINER_NAME`, which the "Launch Docker smoke harness" step writes to `$GITHUB_ENV` only *after* the harness returns. On any failure before that the guard is false, the step does nothing, and the upload reports "No files were found". Below that, `scripts/docker-onboard-smoke.sh` starts the container with `docker run -d --rm`, so the `docker stop` in its EXIT trap deletes the container and its logs together — and a container that crashes on its own is removed the instant its process exits. **Expected behavior** The spec walks the onboarding arc the app actually presents, and proves the company is created, the lead agent is hired, and the first task is seeded and dispatched. When the smoke fails, the run's artifact carries the container's logs. **Steps to reproduce** 1. Run the Release Smoke workflow against a published artifact that carries #12274, or run it locally: `PAPERCLIPAI_VERSION=2026.828.0-canary.3 SMOKE_DETACH=true ./scripts/docker-onboard-smoke.sh` 2. Run `pnpm run test:release-smoke` against that container. 3. The single spec fails at `openOnboarding()` after 20 seconds. 4. In CI, open the run's `release-smoke` artifact. It has no `docker-onboard-smoke.log`. **Paperclip version or commit** `2026.828.0-canary.3` (commit 8316ceb0b). **Deployment mode** Docker. **Installation method** npm / pnpm global install (the container runs `npx paperclipai@`). **Node.js version** v24.20.0 inside the container. **Relevant logs or output** ``` Running 1 test using 1 worker ✓ 1 [chromium] › tests/release-smoke/docker-auth-onboarding.spec.ts:76:3 › Docker authenticated onboarding smoke › logs in, completes onboarding, and hires the lead agent (7.0s) 1 passed (8.7s) ``` That is the result after this change. Before it, the same command failed at the first wait, as quoted above. ## What Changed - `tests/release-smoke/docker-auth-onboarding.spec.ts` now follows the current arc. It signs in, opens `/onboarding`, names the organization and presses "Continue" (which creates the company and routes straight to the agent step, because onboarding no longer asks for a mission), names the lead and presses "Next", presses "Connect" on the default adapter to hire, then presses "Get started" to launch. - The spec addresses controls by role and accessible name, or by id where one exists (`#onboarding-agent-name`). Step 1's field has no id and no associated label, so it is found as the wizard's only text box rather than by its placeholder copy. - The spec asserts the hired agent's role is `general`, which is what the arc files every onboarding hire under. Every other API assertion is unchanged. - The spec navigates to `/onboarding` explicitly and drops any saved onboarding draft first, so it can run twice against one instance. The suite retries once in CI. It still asserts that a company-less board routes sign-in into onboarding, guarded on the board actually being empty. - `scripts/docker-onboard-smoke.sh` accepts `SMOKE_CONTAINER_NAME`, drops `--rm`, removes the container itself, and dumps `docker logs` to `SMOKE_LOG_FILE` before the teardown. - `.github/workflows/release-smoke.yml` pins the container name in the job's `env`, so every `always()` step has it before anything runs. The capture step refreshes the log from a live container when there is one, keeps the harness's dump when there is not, and writes a one-line explanation when there is neither. The upload's paths are literals, and `if-no-files-found: error` makes a broken diagnostics path fail rather than warn. - `scripts/docker-onboard-smoke.test.mjs` pins that wiring. It is added to `test:release-registry`, which runs on every pull request. - `doc/DOCKER.md` documents `SMOKE_CONTAINER_NAME` and `SMOKE_LOG_FILE`. ## Verification The spec was run against a real container built from the published `2026.828.0-canary.3` artifact, exactly as the workflow runs it. ```sh SMOKE_CONTAINER_NAME=release-smoke-onboard \ HOST_PORT=3232 DATA_DIR=/smoke-data \ PAPERCLIPAI_VERSION=2026.828.0-canary.3 \ SMOKE_READY_TIMEOUT_SECONDS=420 SMOKE_DETACH=true \ SMOKE_METADATA_FILE=/release-smoke.env \ SMOKE_LOG_FILE=/docker-onboard-smoke.log \ ./scripts/docker-onboard-smoke.sh PAPERCLIP_RELEASE_SMOKE_BASE_URL=http://localhost:3232 \ PAPERCLIP_RELEASE_SMOKE_EMAIL=smoke-admin@paperclip.local \ PAPERCLIP_RELEASE_SMOKE_PASSWORD=paperclip-smoke-password \ PAPERCLIP_PLAYWRIGHT_CHANNEL=chrome \ pnpm run test:release-smoke ``` ``` Running 1 test using 1 worker ✓ 1 [chromium] › tests/release-smoke/docker-auth-onboarding.spec.ts:76:3 › Docker authenticated onboarding smoke › logs in, completes onboarding, and hires the lead agent (7.0s) 1 passed (8.7s) ``` The same command was run a second time against the same, now non-empty, instance. That covers the retry path, and it also passes. The log capture was verified by making the container die during startup: ```sh PAPERCLIPAI_VERSION=0.0.0-no-such-version \ SMOKE_CONTAINER_NAME=release-smoke-onboard SMOKE_LOG_FILE=/fail.log \ ./scripts/docker-onboard-smoke.sh ``` `/fail.log` was written and carried the cause: ``` npm error code ETARGET npm error notarget No matching version found for paperclipai@0.0.0-no-such-version. ``` The container was removed afterwards. On `master` this file is never written, because `--rm` deletes the container the moment its process exits. The workflow's capture step was run by hand against three states: a live container (258 lines), a removed container with the harness's dump already on disk (258 lines kept), and neither (a one-line explanation). Unit coverage: ```sh pnpm run test:release-registry # 93 tests, 93 pass ``` Nothing under `ui/` changed, so `pnpm --filter @paperclipai/ui typecheck` was not required. `tests/release-smoke` is outside the TypeScript project references; Playwright compiles it at run time, which the runs above did three times. ## Risks Low risk. Nothing ships to users. The change touches one Playwright spec, one smoke script, and one workflow. Points worth a reviewer's attention: - **This suite gates every nightly and beta, and it runs only post-merge.** `smoke_nightly` and `smoke_beta` both call `release-smoke.yml`, and no pull request runs it. Drift between the wizard and this spec is therefore invisible until a release is already blocked, which is how this bug reached a release train. I think the arc deserves an earlier check. The cheapest version is the one added here: `scripts/docker-onboard-smoke.test.mjs` runs on every pull request and pins the harness wiring. The full container smoke is too slow for the pull request path, but a UI-level test of the arc's step sequence would catch exactly this class of drift, and `ui/src/components/OnboardingWizard.step.test.tsx` is already the right home for it. I did not add it here, to keep this change to the repair. - **Dropping `--rm`.** The container is now removed by the script's cleanup instead of by Docker. The script already ran `docker rm -f` before starting, and the workflow's final step removes it too, so a leaked container is cleaned up on the next run either way. A developer who kills the script with `SIGKILL` will leave a stopped container behind, where previously they would not. - **`if-no-files-found: error` on the upload.** The capture step now always writes the log file, so the upload always has at least one path to match. If that ever stops being true, the job fails instead of warning. That is deliberate. - **The spec drops the saved onboarding draft before it walks.** A stale draft makes step 1 skip company creation and hire into the previous run's company. That state only exists when the spec runs twice against one instance. A fresh release-smoke container never has it. ## Model Used Claude (Anthropic), Claude Opus, 1M context, extended thinking, agentic tool use via Claude Code. The container, the Playwright runs, and the failure injection were driven as real commands on a local Docker host. ## Checklist - [x] I have included a thinking path that traces from project context to this change - [x] I have specified the model used (with version and capability details) - [x] I have checked ROADMAP.md and confirmed this PR does not duplicate planned core work - [x] I have searched GitHub for duplicate or related PRs and linked them above - [x] I have either (a) linked existing issues with `Fixes: #` / `Closes #` / `Refs #` OR (b) described the issue in-PR following the relevant issue template - [x] I have not referenced internal/instance-local Paperclip issues or links (only public GitHub `#NNN` / `github.com/paperclipai/paperclip` URLs) - [x] My branch name describes the change (e.g. `docs/...`, `fix/...`) and contains no internal Paperclip ticket id or instance-derived details - [x] I have run tests locally and they pass - [x] I have added or updated tests where applicable - [x] I have updated relevant documentation to reflect my changes - [x] I have considered and documented any risks above - [x] All Paperclip CI gates are green - [x] Greptile is 5/5 with no open P2s, recommendations, or follow-ups - [x] I will address all Greptile and reviewer comments before requesting merge --- .github/workflows/release-smoke.yml | 41 ++-- doc/DOCKER.md | 2 + package.json | 2 +- scripts/docker-onboard-smoke.sh | 53 ++++- scripts/docker-onboard-smoke.test.mjs | 106 ++++++++++ .../docker-auth-onboarding.spec.ts | 194 +++++++++++------- 6 files changed, 307 insertions(+), 91 deletions(-) create mode 100644 scripts/docker-onboard-smoke.test.mjs 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 `