From 0965549ee2d3d5ffc43ac045565eccbd15bb01b3 Mon Sep 17 00:00:00 2001 From: Algis Dumbris Date: Sun, 23 Aug 2026 21:15:54 +0300 Subject: [PATCH 1/5] ci(release): advisory Playwright web-UI sweep on tag builds (QA gate T2) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Spec 081 T2. The release QA gate now runs the Playwright Web UI sweep on every tag build (release.yml / prerelease.yml already call the reusable gate), against the Web UI served by the candidate binary with a live mcpfixture stdio upstream. Advisory by design, enforced twice: `continue-on-error: true` on the job (a failing job inside a reusable workflow would otherwise become the workflow_call conclusion the publishers gate on), and a non-blocking manifest entry `advisory/web-ui-sweep` (renamed from `reserved/web-ui-sweep`) whose failures land in `advisory_failures` via a new `release-gate run-suite --advisory` flag. Promotion to blocking is Blocking:true + dropping continue-on-error. The sweep itself is now committed (e2e/web-ui-sweep: servers list, server detail + security tab, tools page + search, activity log, settings; fails on uncaught page exceptions) and scripts/run-web-smoke.sh — previously dead, it ran a spec file that lives only in a gitignored dir — is the single launcher for both the manual run and the CI job. --- .github/workflows/release-qa-gate.yml | 80 ++++++++++- ROADMAP.md | 7 +- cmd/release-gate/main.go | 5 +- cmd/release-gate/suite.go | 11 +- cmd/release-gate/suite_test.go | 70 ++++++++++ cmd/release-gate/webui_sweep_audit_test.go | 104 ++++++++++++++ cmd/release-gate/workflow_audit_test.go | 14 +- docs/development/release-gate.md | 46 +++++-- docs/development/web-ui-verification.md | 14 ++ e2e/web-ui-sweep/package.json | 13 ++ e2e/web-ui-sweep/playwright.config.ts | 39 ++++++ e2e/web-ui-sweep/web-ui-sweep.spec.ts | 104 ++++++++++++++ internal/gatereport/gatereport.go | 19 ++- internal/gatereport/gatereport_test.go | 64 ++++++++- roadmap.yaml | 3 +- scripts/run-web-smoke.sh | 151 +++++++++++++-------- 16 files changed, 657 insertions(+), 87 deletions(-) create mode 100644 cmd/release-gate/suite_test.go create mode 100644 cmd/release-gate/webui_sweep_audit_test.go create mode 100644 e2e/web-ui-sweep/package.json create mode 100644 e2e/web-ui-sweep/playwright.config.ts create mode 100644 e2e/web-ui-sweep/web-ui-sweep.spec.ts diff --git a/.github/workflows/release-qa-gate.yml b/.github/workflows/release-qa-gate.yml index 953fa4f4f..d654cf757 100644 --- a/.github/workflows/release-qa-gate.yml +++ b/.github/workflows/release-qa-gate.yml @@ -16,9 +16,13 @@ name: Release QA Gate # manually against any ref (workflow_dispatch — a dry run that publishes # nothing and never counts as qualification for a later tag, FR-001a). # -# T2/T3/T4 extension slots (reserved manifest entries, recorded as +# T2 landed as an ADVISORY entry — it runs on every tag build and is reported, +# but cannot block publication (`continue-on-error` + a non-blocking manifest +# entry): +# advisory/web-ui-sweep — T2 Playwright sweep against the candidate +# +# T3/T4 extension slots (reserved manifest entries, recorded as # not-run/"not-implemented-yet" until their stage lands): -# reserved/web-ui-sweep — T2 Playwright sweep against the candidate # reserved/macos-smoke — T3 macOS tray smoke (advisory, FR-019/020) # reserved/surface-consistency — T4 REST/CLI/Web-UI/tray state agreement # See docs/development/release-gate.md. @@ -374,6 +378,76 @@ jobs: retention-days: 1 if-no-files-found: warn + # --------------------------------------------------------------------------- + # T2 (US3, FR-016): Playwright Web UI sweep against the Web UI served by the + # CANDIDATE binary (embedded frontend, not a dev server), with a live stdio + # mcpfixture upstream so the servers/tools screens have real data. + # + # ADVISORY, deliberately: `continue-on-error: true` keeps a red sweep out of + # this reusable workflow's conclusion, so a publisher whose job `needs:` the + # gate still publishes. The sweep is nonetheless REPORTED — the driver runs it + # with --advisory, which records `advisory/web-ui-sweep` as `advisory-fail`, + # and the verdict job lists it under advisory_failures. Promote to blocking by + # flipping that manifest entry to Blocking:true and dropping continue-on-error. + # + # Setup is not reinvented here: scripts/run-web-smoke.sh is the same launcher + # used by hand (docs/development/web-ui-verification.md) — it boots a + # throwaway instance, installs Chromium, and runs e2e/web-ui-sweep. + # --------------------------------------------------------------------------- + web-ui-sweep: + name: Web UI sweep (advisory) + needs: build-candidate + runs-on: ubuntu-latest + timeout-minutes: 20 + continue-on-error: true # advisory: never blocks the tag (FR-019 pattern) + steps: + - name: Checkout + uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + with: + ref: ${{ inputs.ref || github.sha }} + + - name: Set up Node.js # the sweep installs @playwright/test + Chromium + uses: actions/setup-node@820762786026740c76f36085b0efc47a31fe5020 # v7.0.0 + with: + node-version: ${{ env.NODE_VERSION }} + + - name: Download candidate binaries + uses: actions/download-artifact@3e5f45b2cfb9172054b4087a40e8e0b5a5461e7c # v8.0.1 + with: + name: gate-candidate + path: dist-bin + + - name: Stage binaries + run: chmod +x dist-bin/* + + - name: Run Web UI sweep + env: + MCPPROXY_BINARY_PATH: ${{ github.workspace }}/dist-bin/mcpproxy + MCPPROXY_FIXTURE_PATH: ${{ github.workspace }}/dist-bin/mcpfixture + ARTIFACT_DIR: ${{ github.workspace }}/tmp/web-smoke-artifacts + run: | + ./dist-bin/release-gate run-suite \ + --name advisory/web-ui-sweep --advisory --report-dir "${GATE_REPORT_DIR}" \ + -- bash scripts/run-web-smoke.sh + + - name: Upload Playwright report + if: always() + uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7.0.1 + with: + name: web-ui-sweep-playwright-report + path: tmp/web-smoke-artifacts/ + retention-days: 14 + if-no-files-found: warn + + - name: Upload report fragment + if: always() + uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7.0.1 + with: + name: gate-fragment-web-ui-sweep + path: ${{ env.GATE_REPORT_DIR }}/ + retention-days: 1 + if-no-files-found: warn + # --------------------------------------------------------------------------- # Merge every fragment against the hardcoded gatereport manifest → one # verdict. Runs even if upstream jobs failed (if: always()) so a missing @@ -383,7 +457,7 @@ jobs: # --------------------------------------------------------------------------- verdict: name: Gate verdict - needs: [build-candidate, suite-api-e2e, suite-race, suite-scan-eval, matrix-invariants] + needs: [build-candidate, suite-api-e2e, suite-race, suite-scan-eval, matrix-invariants, web-ui-sweep] if: always() runs-on: ubuntu-latest timeout-minutes: 10 diff --git a/ROADMAP.md b/ROADMAP.md index 4643c721d..c08ca0a79 100644 --- a/ROADMAP.md +++ b/ROADMAP.md @@ -105,15 +105,17 @@ graph LR release_qa_gate_matrix --> release_qa_gate_consistency classDef done fill:#1f7a1f,stroke:#0d3d0d,color:#ffffff; + classDef in_review fill:#9a6700,stroke:#5c3d00,color:#ffffff; classDef todo fill:#6e7781,stroke:#3d4248,color:#ffffff; class release_qa_gate_matrix done; - class release_qa_gate_playwright,release_qa_gate_macos,release_qa_gate_consistency todo; + class release_qa_gate_playwright in_review; + class release_qa_gate_macos,release_qa_gate_consistency todo; ``` | Task | Status | Refs | | --- | --- | --- | | T1: tag-blocking release-gate workflow: server-type matrix (stdio/http/sse/docker/oauth) + invariants (activity-log/request-id, token+telemetry counters, quarantine flow, reconnect, upgrade-in-place), publish jobs gated on the verdict, scan-eval unconditional on tags | 🟢 Done | #819 | -| T2: wire the Playwright Web UI sweep into the gate (currently manual-trigger only) | ⚪ Todo | — | +| T2: wire the Playwright Web UI sweep into the gate (currently manual-trigger only) | 🟡 In review | — | | T3: macOS app smoke on a macos runner, advisory until 3 consecutive passes (today zero CI automation for the tray app) | ⚪ Todo | — | | T4: surface-state consistency check (tray/Web UI/CLI agree with core on server states) | ⚪ Todo | — | @@ -801,3 +803,4 @@ Legend: `shipped` ≥95% checked · `in-flight` 1–94% · `drafted` 0% · `—` | [097-stored-scripts](./specs/097-stored-scripts/) | `in-flight` | 13/14 (93%) | | [098-tools-preflight](./specs/098-tools-preflight/) | `in-flight` | 26/33 (79%) | | [099-describe-check-mode](./specs/099-describe-check-mode/) | `in-flight` | 9/10 (90%) | +| [100-prompt-rugpull-baseline](./specs/100-prompt-rugpull-baseline/) | — | — | diff --git a/cmd/release-gate/main.go b/cmd/release-gate/main.go index 5a7dc0af4..e14b4cbbe 100644 --- a/cmd/release-gate/main.go +++ b/cmd/release-gate/main.go @@ -76,7 +76,7 @@ usage: [--work-dir DIR] [--cell-timeout 300s] release-gate invariants --state-file FILE --report-dir DIR [--prev-binary PATH] [--skip-upgrade] \ [--upgrade-repo owner/repo] [--keep-core] - release-gate run-suite --name suite/api-e2e --report-dir DIR -- CMD [ARGS...] + release-gate run-suite --name suite/api-e2e --report-dir DIR [--advisory] -- CMD [ARGS...] release-gate report --report-dir DIR [--out gate-report.json] [--summary summary.md] `) } @@ -128,6 +128,7 @@ func cmdRunSuite(ctx context.Context, args []string) (bool, error) { fs := flag.NewFlagSet("run-suite", flag.ExitOnError) name := fs.String("name", "", "manifest entry name (e.g. suite/api-e2e)") reportDir := fs.String("report-dir", "", "directory for report fragments (required)") + advisory := fs.Bool("advisory", false, "non-blocking entry: record failures as advisory-fail") if err := fs.Parse(args); err != nil { return false, err } @@ -138,7 +139,7 @@ func cmdRunSuite(ctx context.Context, args []string) (bool, error) { if len(cmdArgs) == 0 { return false, fmt.Errorf("no command given after flags (use: run-suite --name N --report-dir D -- CMD ARGS)") } - return runSuite(ctx, *name, *reportDir, cmdArgs) + return runSuite(ctx, *name, *reportDir, *advisory, cmdArgs) } func mustAbs(p string) string { diff --git a/cmd/release-gate/suite.go b/cmd/release-gate/suite.go index 0685109b2..418b68e3c 100644 --- a/cmd/release-gate/suite.go +++ b/cmd/release-gate/suite.go @@ -14,7 +14,13 @@ import ( // test-api-e2e.sh, go race tests, scan-eval --gate) and records its outcome // as a report fragment. The command's output streams through so CI logs stay // useful. -func runSuite(ctx context.Context, name, reportDir string, cmdArgs []string) (bool, error) { +// +// advisory marks a non-blocking entry (Spec 081 T2 Web UI sweep, T3 macOS +// smoke): its failures are recorded as `advisory-fail` so the merged report +// distinguishes "went red but cannot block the tag" from a real blocking +// failure. The returned ok is false either way — the CI job still goes red +// (under continue-on-error) so a maintainer sees it. +func runSuite(ctx context.Context, name, reportDir string, advisory bool, cmdArgs []string) (bool, error) { frag := &gatereport.Fragment{Name: name, StartedAt: time.Now().UTC()} cmd := exec.CommandContext(ctx, cmdArgs[0], cmdArgs[1:]...) cmd.Stdout = os.Stdout @@ -26,6 +32,9 @@ func runSuite(ctx context.Context, name, reportDir string, cmdArgs []string) (bo frag.DurationMS = frag.FinishedAt.Sub(frag.StartedAt).Milliseconds() if err != nil { frag.Status = gatereport.StatusFail + if advisory { + frag.Status = gatereport.StatusAdvisoryFail + } frag.Reason = fmt.Sprintf("suite command failed: %v", err) frag.Classification = gatereport.ClassificationProduct } else { diff --git a/cmd/release-gate/suite_test.go b/cmd/release-gate/suite_test.go new file mode 100644 index 000000000..8ed13dccd --- /dev/null +++ b/cmd/release-gate/suite_test.go @@ -0,0 +1,70 @@ +package main + +import ( + "context" + "encoding/json" + "os" + "path/filepath" + "testing" + + "github.com/smart-mcp-proxy/mcpproxy-go/internal/gatereport" +) + +func readFragment(t *testing.T, dir, name string) gatereport.Fragment { + t.Helper() + data, err := os.ReadFile(filepath.Join(dir, gatereport.FragmentFileName(name))) + if err != nil { + t.Fatalf("read fragment %s: %v", name, err) + } + var frag gatereport.Fragment + if err := json.Unmarshal(data, &frag); err != nil { + t.Fatalf("parse fragment %s: %v", name, err) + } + return frag +} + +// A failing advisory suite must be recorded as advisory-fail (never plain +// fail), so the merged report reads honestly: the check ran, it went red, and +// it did not block the tag (Spec 081 T2/FR-019). +func TestRunSuite_AdvisoryFailure_RecordsAdvisoryFail(t *testing.T) { + dir := t.TempDir() + ok, err := runSuite(context.Background(), gatereport.EntryAdvisoryWebUISweep, dir, true, + []string{"sh", "-c", "exit 3"}) + if err != nil { + t.Fatalf("runSuite returned an error: %v", err) + } + if ok { + t.Error("a failing suite must report ok=false even when advisory (the job goes red under continue-on-error)") + } + frag := readFragment(t, dir, gatereport.EntryAdvisoryWebUISweep) + if frag.Status != gatereport.StatusAdvisoryFail { + t.Errorf("status=%s want %s", frag.Status, gatereport.StatusAdvisoryFail) + } + if frag.Reason == "" { + t.Error("a non-pass fragment must carry a reason (FR-004)") + } +} + +func TestRunSuite_AdvisorySuccess_RecordsPass(t *testing.T) { + dir := t.TempDir() + ok, err := runSuite(context.Background(), gatereport.EntryAdvisoryWebUISweep, dir, true, + []string{"sh", "-c", "exit 0"}) + if err != nil || !ok { + t.Fatalf("runSuite ok=%v err=%v, want true/nil", ok, err) + } + if frag := readFragment(t, dir, gatereport.EntryAdvisoryWebUISweep); frag.Status != gatereport.StatusPass { + t.Errorf("status=%s want %s", frag.Status, gatereport.StatusPass) + } +} + +// Blocking suites keep the original behaviour: a failure is a plain fail. +func TestRunSuite_BlockingFailure_RecordsFail(t *testing.T) { + dir := t.TempDir() + if _, err := runSuite(context.Background(), gatereport.EntrySuiteAPIE2E, dir, false, + []string{"sh", "-c", "exit 1"}); err != nil { + t.Fatalf("runSuite returned an error: %v", err) + } + if frag := readFragment(t, dir, gatereport.EntrySuiteAPIE2E); frag.Status != gatereport.StatusFail { + t.Errorf("status=%s want %s", frag.Status, gatereport.StatusFail) + } +} diff --git a/cmd/release-gate/webui_sweep_audit_test.go b/cmd/release-gate/webui_sweep_audit_test.go new file mode 100644 index 000000000..941b52035 --- /dev/null +++ b/cmd/release-gate/webui_sweep_audit_test.go @@ -0,0 +1,104 @@ +package main + +import ( + "path/filepath" + "strings" + "testing" + + "gopkg.in/yaml.v3" + + "github.com/smart-mcp-proxy/mcpproxy-go/internal/gatereport" +) + +// gateWorkflowPath is the reusable gate workflow the publishers call. go test +// runs with cmd/release-gate as the working directory. +var gateWorkflowPath = filepath.Join("..", "..", ".github", "workflows", "release-qa-gate.yml") + +const webUISweepJob = "web-ui-sweep" + +// TestWebUISweepJobIsAdvisory is the Spec 081 T2 wiring audit: the Playwright +// Web UI sweep must run inside the gate (so it runs on every tag build, since +// release.yml / prerelease.yml call this workflow) while being incapable of +// blocking the release. +// +// Advisory is enforced structurally: `continue-on-error: true` on the job. A +// failing job inside a reusable workflow makes the whole workflow_call +// conclusion `failure`, which the publishers' `needs:` would treat as a red +// gate — continue-on-error is what keeps the sweep out of that verdict. +func TestWebUISweepJobIsAdvisory(t *testing.T) { + wf := parseWorkflow(t, gateWorkflowPath) + + sweep, ok := wf.Jobs[webUISweepJob] + if !ok { + t.Fatalf("release-qa-gate.yml has no %q job — the Playwright sweep does not run on tag builds", webUISweepJob) + } + if strings.TrimSpace(sweep.ContinueOnError.Value) != "true" { + t.Errorf("job %q must set `continue-on-error: true` (got %q) or a red sweep would block the release", + webUISweepJob, sweep.ContinueOnError.Value) + } + if sweep.disabled() { + t.Errorf("job %q is statically disabled — it would never run on a tag", webUISweepJob) + } + + // The verdict job must wait for the sweep, otherwise the fragment can land + // after the merge and the report would show a missing entry every run. + verdict, ok := wf.Jobs["verdict"] + if !ok { + t.Fatal("release-qa-gate.yml has no verdict job") + } + var needsSweep bool + for _, n := range verdict.Needs { + if n == webUISweepJob { + needsSweep = true + } + } + if !needsSweep { + t.Errorf("verdict job must list %q in needs (got %v) so the sweep fragment is merged", webUISweepJob, verdict.Needs) + } +} + +// TestWebUISweepJobReportsAndUploads pins the two observable outputs of the +// advisory job: the manifest fragment name the merger expects, and the +// Playwright HTML report artifact a maintainer reads after a red sweep. +func TestWebUISweepJobReportsAndUploads(t *testing.T) { + wf := parseWorkflow(t, gateWorkflowPath) + sweep := wf.Jobs[webUISweepJob] + + var runs, uploads string + for _, s := range sweep.Steps { + runs += s.Run + "\n" + if strings.Contains(s.Uses, "upload-artifact") { + uploads += yamlFlatten(&s.With) + "\n" + } + } + + if !strings.Contains(runs, gatereport.EntryAdvisoryWebUISweep) { + t.Errorf("the sweep job must record its outcome under the manifest entry %q; run steps: %s", + gatereport.EntryAdvisoryWebUISweep, runs) + } + if !strings.Contains(runs, "--advisory") { + t.Errorf("the sweep job must pass --advisory to release-gate run-suite so a failure is recorded as advisory-fail; run steps: %s", runs) + } + if !strings.Contains(uploads, "playwright-report") { + t.Errorf("the sweep job must upload the Playwright HTML report artifact; upload steps: %s", uploads) + } +} + +// yamlFlatten renders a `with:` mapping node as a flat string for substring +// assertions (the audit only cares whether a path/name appears at all). +func yamlFlatten(n *yaml.Node) string { + var b strings.Builder + var walk func(node *yaml.Node) + walk = func(node *yaml.Node) { + if node == nil { + return + } + b.WriteString(node.Value) + b.WriteString(" ") + for _, c := range node.Content { + walk(c) + } + } + walk(n) + return b.String() +} diff --git a/cmd/release-gate/workflow_audit_test.go b/cmd/release-gate/workflow_audit_test.go index ed416983c..2fef486bf 100644 --- a/cmd/release-gate/workflow_audit_test.go +++ b/cmd/release-gate/workflow_audit_test.go @@ -66,15 +66,17 @@ type workflow struct { } type job struct { - Needs needsList `yaml:"needs"` - Uses string `yaml:"uses"` - If yaml.Node `yaml:"if"` - Steps []step `yaml:"steps"` + Needs needsList `yaml:"needs"` + Uses string `yaml:"uses"` + If yaml.Node `yaml:"if"` + ContinueOnError yaml.Node `yaml:"continue-on-error"` + Steps []step `yaml:"steps"` } type step struct { - Uses string `yaml:"uses"` - Run string `yaml:"run"` + Uses string `yaml:"uses"` + Run string `yaml:"run"` + With yaml.Node `yaml:"with"` } // needsList decodes `needs:` which may be a scalar (`needs: build`) or a diff --git a/docs/development/release-gate.md b/docs/development/release-gate.md index 31164c266..c82533d65 100644 --- a/docs/development/release-gate.md +++ b/docs/development/release-gate.md @@ -33,6 +33,7 @@ fragments against a hardcoded manifest and exits per the verdict. | `suite-race` | `suite/unit-race`, `suite/server-race` | 25 min | `go test -race ./internal/...` + `go test -tags server -race ./internal/serveredition/...`. | | `suite-scan-eval` | `suite/scan-eval` | 10 min | `go run ./cmd/scan-eval --gate --min-recall 0.90 --max-fp 0.05` over the detect corpus — runs on **every** tag regardless of changed paths (FR-015). | | `matrix-invariants` | `matrix/{stdio,http,sse,docker,oauth}`, `invariant/{activity-request-id,counters,quarantine-flow,upgrade-in-place}` | 20 min | Boots the candidate against five local fixture upstreams (connect → list → call → kill/reconnect) and asserts the US2 invariants against the live instance. | +| `web-ui-sweep` | — (advisory `advisory/web-ui-sweep`, T2) | 20 min | Playwright sweep over the Web UI **served by the candidate binary**, with a live stdio fixture upstream. `continue-on-error: true` — see [Web UI sweep](#web-ui-sweep-t2--advisory). | | `verdict` | (merges all) | 10 min | `release-gate report` → `gate-report.json` + `$GITHUB_STEP_SUMMARY`; its exit code **is** the gate verdict. | Every job uploads its fragment with `if: always()`, so a job that dies before @@ -101,17 +102,44 @@ scripts/gate/build-fixture-image.sh ./release-gate report --report-dir ./gate-report ``` -## Extension slots (T2 / T3 / T4) - -The manifest reserves three non-blocking slots, recorded as +## Web UI sweep (T2) — advisory + +The `web-ui-sweep` job runs the Playwright sweep +([`e2e/web-ui-sweep`](https://github.com/smart-mcp-proxy/mcpproxy-go/tree/main/e2e/web-ui-sweep), +see [Web UI verification](web-ui-verification.md)) against the Web UI **served by +the candidate binary** — embedded frontend, never a dev server — with a live +`mcpfixture` stdio upstream so the servers/tools screens have real data. It +covers the servers list, server detail (+ security tab), tools page and its +search, activity log, and settings, and fails on uncaught page exceptions. + +Setup is not duplicated in YAML: the job calls +[`scripts/run-web-smoke.sh`](https://github.com/smart-mcp-proxy/mcpproxy-go/blob/main/scripts/run-web-smoke.sh), +the same launcher used by hand (`./scripts/run-web-smoke.sh --show-report`), +passing `MCPPROXY_BINARY_PATH` / `MCPPROXY_FIXTURE_PATH` from the downloaded +`gate-candidate` artifact. + +It is **advisory**, on purpose while the sweep earns its flake record: + +- the job sets `continue-on-error: true`, so its failure never becomes the + reusable workflow's conclusion — publishers whose `needs:` list the gate still + publish; +- the driver runs with `--advisory`, so a red sweep is recorded as + `advisory-fail` under the non-blocking manifest entry `advisory/web-ui-sweep` + and listed in `advisory_failures` — reported, never silent; +- the Playwright HTML report + server log are uploaded as the + `web-ui-sweep-playwright-report` artifact (14 days) for post-mortem. + +**Promotion to blocking** is a two-line change: set `Blocking: true` on the +`advisory/web-ui-sweep` manifest entry and drop `continue-on-error` from the job +(the FR-016 end state). Do it once the sweep has passed on three consecutive +release tags with no flaky or infrastructure failure — the same criterion T3 +uses (FR-021). + +## Extension slots (T3 / T4) + +The manifest reserves two non-blocking slots, recorded as `not-run` / `not-implemented-yet` until their stage lands: -- **`reserved/web-ui-sweep` (T2)** — run the existing Playwright Web UI sweep - (`docs/development/web-ui-verification.md`) against the candidate binary's - **embedded** frontend, with the matrix fixtures as its upstream data - (US3, FR-016/017). Add a `web-ui-sweep` job that downloads the `gate-candidate` - artifact, serves it, runs the sweep, and writes a `reserved/web-ui-sweep` - fragment; then flip that manifest entry to blocking. - **`reserved/macos-smoke` (T3)** — a macOS-runner job that launches the tray against a running core and uses the `mcpproxy-ui-test` accessibility primitives to assert presence, menu items, and state agreement (US4, diff --git a/docs/development/web-ui-verification.md b/docs/development/web-ui-verification.md index 49f06799a..20cb463c1 100644 --- a/docs/development/web-ui-verification.md +++ b/docs/development/web-ui-verification.md @@ -8,6 +8,20 @@ description: "Playwright sweep and HTML report workflow for verifying changes to When you modify the Web UI (any Vue file under `frontend/src/`), verify it end-to-end with a Playwright sweep that captures screenshots and packages them into a self-contained HTML report. This is the same workflow used to verify Spec 046 v2 — see `specs/046-local-first-onboarding/verification/` for a worked example. +## The standing sweep (one command) + +The core-screen sweep is committed and scripted — run it before you hand-roll anything: + +```bash +./scripts/run-web-smoke.sh --show-report # boots a throwaway instance, runs e2e/web-ui-sweep +``` + +The launcher builds `./mcpproxy` if needed, serves a throwaway instance on `127.0.0.1:18080`, installs Chromium, and runs [`e2e/web-ui-sweep/web-ui-sweep.spec.ts`](https://github.com/smart-mcp-proxy/mcpproxy-go/blob/main/e2e/web-ui-sweep/web-ui-sweep.spec.ts) — servers list, server detail (+ security tab), tools page and search, activity log, settings — failing on uncaught page exceptions. Pass `MCPPROXY_FIXTURE_PATH=$(go build -o /tmp/mcpfixture ./cmd/mcpfixture && echo /tmp/mcpfixture)` to register a live stdio upstream so the server- and tool-dependent checks run instead of skipping. The HTML report lands in `tmp/web-smoke-artifacts/playwright-report/`. + +The release QA gate runs this exact script on every tag as its **advisory** `web-ui-sweep` job — see [Release Gate](release-gate.md#web-ui-sweep-t2--advisory). Extend the committed sweep when you add a screen worth guarding on releases; use the ad-hoc pattern below for the deeper, spec-specific verification that ships beside a spec. + +## Ad-hoc, spec-specific verification + The pattern, in order: 1. **Stand up a fresh mcpproxy.** Use a throwaway data-dir so persisted state doesn't bleed between runs: diff --git a/e2e/web-ui-sweep/package.json b/e2e/web-ui-sweep/package.json new file mode 100644 index 000000000..0fe8bee27 --- /dev/null +++ b/e2e/web-ui-sweep/package.json @@ -0,0 +1,13 @@ +{ + "name": "mcpproxy-web-ui-sweep", + "version": "1.0.0", + "private": true, + "description": "Playwright Web UI sweep driven by scripts/run-web-smoke.sh (manual) and the release QA gate (advisory job)", + "scripts": { + "test": "playwright test", + "install-browsers": "playwright install chromium" + }, + "devDependencies": { + "@playwright/test": "^1.49.0" + } +} diff --git a/e2e/web-ui-sweep/playwright.config.ts b/e2e/web-ui-sweep/playwright.config.ts new file mode 100644 index 000000000..b04d19f7d --- /dev/null +++ b/e2e/web-ui-sweep/playwright.config.ts @@ -0,0 +1,39 @@ +import { defineConfig, devices } from '@playwright/test' + +// Web UI sweep configuration (docs/development/web-ui-verification.md). +// +// The sweep drives the Web UI SERVED BY A REAL mcpproxy binary (embedded +// frontend, never a dev server), so everything here is parameterised by the +// environment the launcher (scripts/run-web-smoke.sh) exports: +// MCPPROXY_BASE_URL — the running instance, e.g. http://127.0.0.1:18080 +// MCPPROXY_API_KEY — API key of that instance (Web UI accepts ?apikey=) +// SWEEP_REPORT_DIR — where the self-contained HTML report is written +// PW_CHROMIUM — optional explicit Chromium binary (local macOS runs) +const reportDir = process.env.SWEEP_REPORT_DIR || './playwright-report' + +export default defineConfig({ + testDir: '.', + timeout: 45_000, + expect: { timeout: 15_000 }, + fullyParallel: false, + workers: 1, + forbidOnly: !!process.env.CI, + // One retry: the sweep is advisory in CI, and a single flake should not cost + // a maintainer an investigation. Genuine breakage fails both attempts. + retries: process.env.CI ? 1 : 0, + reporter: [ + ['list'], + ['html', { outputFolder: reportDir, open: 'never' }], + ], + use: { + ...devices['Desktop Chrome'], + headless: true, + viewport: { width: 1440, height: 900 }, + screenshot: 'only-on-failure', + video: 'off', + trace: 'retain-on-failure', + ...(process.env.PW_CHROMIUM + ? { launchOptions: { executablePath: process.env.PW_CHROMIUM } } + : {}), + }, +}) diff --git a/e2e/web-ui-sweep/web-ui-sweep.spec.ts b/e2e/web-ui-sweep/web-ui-sweep.spec.ts new file mode 100644 index 000000000..60049b148 --- /dev/null +++ b/e2e/web-ui-sweep/web-ui-sweep.spec.ts @@ -0,0 +1,104 @@ +// Web UI sweep — the core-screen pass a maintainer used to run by hand before +// a release (docs/development/web-ui-verification.md). +// +// It drives the Web UI served by a REAL mcpproxy binary with its embedded +// frontend (never a dev server), against a live stdio fixture upstream, and +// covers: servers list, server detail (+ its security tab), tools page and +// its search, activity log, and settings. Uncaught page exceptions fail the +// sweep too — a screen that renders while throwing is not "working". +// +// Launcher: scripts/run-web-smoke.sh (boots the instance, installs Chromium, +// runs this file). The release QA gate calls that same script from its +// advisory `web-ui-sweep` job. +import { test, expect, Page } from '@playwright/test' + +const BASE = process.env.MCPPROXY_BASE_URL || 'http://127.0.0.1:18080' +const KEY = process.env.MCPPROXY_API_KEY || '' +// Name of the fixture upstream the launcher registered, when it registered +// one. Unset ⇒ the sweep sticks to instance-independent structural checks. +const SERVER = process.env.SWEEP_SERVER_NAME || '' + +function url(route: string): string { + const sep = route.includes('?') ? '&' : '?' + return KEY ? `${BASE}/ui${route}${sep}apikey=${encodeURIComponent(KEY)}` : `${BASE}/ui${route}` +} + +/** Uncaught exceptions on the page fail the check that triggered them. */ +function watchPageErrors(page: Page): string[] { + const errors: string[] = [] + page.on('pageerror', (err) => errors.push(String(err))) + return errors +} + +/** + * Navigate to a Web UI route. `networkidle` never settles here — the UI holds + * an SSE channel open — so wait for DOM content plus the expected anchor. + */ +async function goto(page: Page, route: string, anchor: string) { + await page.goto(url(route)) + await page.waitForLoadState('domcontentloaded') + // A fresh instance may open the onboarding wizard over the page. + const closeWizard = page.locator('[data-test="close-wizard"]') + if (await closeWizard.isVisible().catch(() => false)) { + await closeWizard.click() + } + await page.locator(anchor).first().waitFor({ state: 'visible' }) +} + +test('servers list renders the fleet with KPI counters', async ({ page }) => { + const errors = watchPageErrors(page) + await goto(page, '/servers', '[data-test="kpi-card-total"]') + + await expect(page.locator('[data-test="kpi-card-total"]')).toContainText(/\d/) + if (SERVER) { + await expect(page.locator('[data-test="server-card-title"]', { hasText: SERVER }).first()) + .toBeVisible() + } + expect(errors, `uncaught page errors on /servers: ${errors.join(' | ')}`).toHaveLength(0) +}) + +test('server detail opens and exposes the security tab', async ({ page }) => { + test.skip(!SERVER, 'no fixture upstream registered for this sweep') + const errors = watchPageErrors(page) + await goto(page, `/servers/${encodeURIComponent(SERVER)}`, '[data-test="security-tab"]') + + await page.locator('[data-test="security-tab"]').click() + await expect(page.locator('[data-test="server-status-badge"]').first()).toBeVisible() + expect(errors, `uncaught page errors on /servers/${SERVER}: ${errors.join(' | ')}`).toHaveLength(0) +}) + +test('tools page lists upstream tools and search narrows them', async ({ page }) => { + test.skip(!SERVER, 'no fixture upstream registered for this sweep') + const errors = watchPageErrors(page) + await goto(page, '/tools', '[data-test="tools-page"]') + + const rows = page.locator('[data-test="tool-row"]') + // Tool indexing runs in the background after the upstream connects. + await expect.poll(() => rows.count(), { timeout: 30_000 }).toBeGreaterThan(0) + const before = await rows.count() + + await page.locator('[data-test="tools-search"]').fill('echo') + await expect.poll(() => rows.count(), { timeout: 10_000 }).toBeLessThanOrEqual(before) + await expect(rows.first()).toContainText(/echo/i) + expect(errors, `uncaught page errors on /tools: ${errors.join(' | ')}`).toHaveLength(0) +}) + +test('activity log renders and its filter panel expands', async ({ page }) => { + const errors = watchPageErrors(page) + // The compact header is the always-present anchor; the KPI stat cards only + // render once the filter panel is expanded AND a summary has loaded. + await goto(page, '/activity', '[data-test="activity-filters-toggle"]') + + await page.locator('[data-test="activity-filters-toggle"]').click() + await expect(page.locator('[data-test="activity-filter-panel"]')).toBeVisible() + expect(errors, `uncaught page errors on /activity: ${errors.join(' | ')}`).toHaveLength(0) +}) + +test('settings page renders its tabs and security posture', async ({ page }) => { + const errors = watchPageErrors(page) + await goto(page, '/settings', '[data-test="settings-tabs"]') + + await expect(page.locator('[data-test="settings-posture"]')).toBeVisible() + await expect(page.locator('[data-test="setting-secret-api_key"]')).toBeVisible() + expect(errors, `uncaught page errors on /settings: ${errors.join(' | ')}`).toHaveLength(0) +}) diff --git a/internal/gatereport/gatereport.go b/internal/gatereport/gatereport.go index e2fc0aa2b..47888e941 100644 --- a/internal/gatereport/gatereport.go +++ b/internal/gatereport/gatereport.go @@ -6,9 +6,10 @@ // Fragment file into a shared report directory. The merger compares the // fragments against a HARDCODED manifest of expected entries: a missing // fragment for a blocking entry is a FAIL (no silent skips, FR-004), and -// reserved extension slots (web-ui-sweep, macos-smoke, surface-consistency — -// Spec 081 T2-T4) are recorded as `not-run` with reason -// "not-implemented-yet" until their stages land. +// reserved extension slots (macos-smoke, surface-consistency — Spec 081 +// T3/T4) are recorded as `not-run` with reason "not-implemented-yet" until +// their stages land. The T2 Web UI sweep is a live ADVISORY entry: it runs on +// tag builds and its failures are reported without blocking the release. package gatereport import ( @@ -101,13 +102,18 @@ const ( EntrySuiteServerRace = "suite/server-race" EntrySuiteScanEval = "suite/scan-eval" - EntryReservedWebUISweep = "reserved/web-ui-sweep" + // EntryAdvisoryWebUISweep is the Spec 081 T2 Playwright Web UI sweep. It + // runs on every tag build but is ADVISORY: a red sweep is reported (and + // listed under advisory_failures) without blocking publication. + EntryAdvisoryWebUISweep = "advisory/web-ui-sweep" + EntryReservedMacOSSmoke = "reserved/macos-smoke" EntryReservedSurfaceConsistency = "reserved/surface-consistency" ) // Manifest returns the hardcoded expected-entries manifest: 5 matrix cells + -// 4 invariants + 4 assembled suite jobs (FR-003) + 3 reserved slots. +// 4 invariants + 4 assembled suite jobs (FR-003) + 1 advisory entry (the T2 +// Web UI sweep) + 2 reserved slots (T3/T4). func Manifest() []ManifestEntry { return []ManifestEntry{ {Name: EntryMatrixStdio, Blocking: true}, @@ -126,7 +132,8 @@ func Manifest() []ManifestEntry { {Name: EntrySuiteServerRace, Blocking: true}, {Name: EntrySuiteScanEval, Blocking: true}, - {Name: EntryReservedWebUISweep, Blocking: false, Reserved: true}, + {Name: EntryAdvisoryWebUISweep, Blocking: false}, + {Name: EntryReservedMacOSSmoke, Blocking: false, Reserved: true}, {Name: EntryReservedSurfaceConsistency, Blocking: false, Reserved: true}, } diff --git a/internal/gatereport/gatereport_test.go b/internal/gatereport/gatereport_test.go index c9d41d5e1..32994cd84 100644 --- a/internal/gatereport/gatereport_test.go +++ b/internal/gatereport/gatereport_test.go @@ -36,7 +36,7 @@ func TestMerge_AllBlockingPass_VerdictPass_ReservedNotRun(t *testing.T) { t.Fatalf("expected pass verdict, got %s (failures: %v)", r.Verdict, r.BlockingFailures) } // Reserved slots must be recorded, never silently absent (FR-004). - for _, name := range []string{EntryReservedWebUISweep, EntryReservedMacOSSmoke, EntryReservedSurfaceConsistency} { + for _, name := range []string{EntryReservedMacOSSmoke, EntryReservedSurfaceConsistency} { e := entryByName(t, r, name) if e.Status != StatusNotRun || e.Reason != ReasonNotImplemented { t.Errorf("%s: got status=%s reason=%q, want not-run/%s", name, e.Status, e.Reason, ReasonNotImplemented) @@ -119,6 +119,68 @@ func TestMerge_AdvisoryFailOnNonBlockingReservedSlot_DoesNotBlock(t *testing.T) } } +// Spec 081 T2: the Web UI sweep is wired as an ADVISORY entry — it is a real +// manifest entry (no longer a reserved "not-implemented-yet" slot), it never +// blocks the tag, and every non-green outcome (including a missing fragment, +// i.e. the job died before uploading) is still reported as an advisory failure. +func TestManifest_WebUISweep_IsAdvisoryNotReserved(t *testing.T) { + var found bool + for _, m := range Manifest() { + if m.Name != EntryAdvisoryWebUISweep { + continue + } + found = true + if m.Blocking { + t.Errorf("%s must not block the gate (advisory, T2)", m.Name) + } + if m.Reserved { + t.Errorf("%s must no longer be a reserved slot — the sweep job exists", m.Name) + } + } + if !found { + t.Fatalf("manifest is missing the %s entry", EntryAdvisoryWebUISweep) + } +} + +func TestMerge_WebUISweepFailure_IsAdvisoryOnly(t *testing.T) { + for _, status := range []Status{StatusFail, StatusAdvisoryFail} { + frags := passAllBlocking() + for i := range frags { + if frags[i].Name == EntryAdvisoryWebUISweep { + frags[i].Status = status + frags[i].Reason = "sweep spec failed" + } + } + r := Merge(frags) + if !r.Passed() { + t.Fatalf("web-ui sweep %s must not block the gate: %v", status, r.BlockingFailures) + } + if len(r.AdvisoryFailures) != 1 || !strings.Contains(r.AdvisoryFailures[0], EntryAdvisoryWebUISweep) { + t.Errorf("web-ui sweep %s must be reported as an advisory failure, got %v", status, r.AdvisoryFailures) + } + } +} + +func TestMerge_WebUISweepMissingFragment_IsAdvisoryFailureNotBlocking(t *testing.T) { + var kept []Fragment + for _, f := range passAllBlocking() { + if f.Name != EntryAdvisoryWebUISweep { + kept = append(kept, f) + } + } + r := Merge(kept) + if !r.Passed() { + t.Fatalf("a missing web-ui sweep fragment must not block the gate: %v", r.BlockingFailures) + } + e := entryByName(t, r, EntryAdvisoryWebUISweep) + if e.Status != StatusFail || e.Reason != ReasonMissingFragment { + t.Errorf("got status=%s reason=%q, want fail/%q", e.Status, e.Reason, ReasonMissingFragment) + } + if len(r.AdvisoryFailures) != 1 || !strings.Contains(r.AdvisoryFailures[0], EntryAdvisoryWebUISweep) { + t.Errorf("missing sweep fragment must surface as an advisory failure, got %v", r.AdvisoryFailures) + } +} + func TestMerge_UnexpectedFailingFragment_Blocks(t *testing.T) { frags := append(passAllBlocking(), Fragment{Name: "rogue/check", Status: StatusFail, Reason: "boom"}) r := Merge(frags) diff --git a/roadmap.yaml b/roadmap.yaml index b9b15f01a..d6115edc5 100644 --- a/roadmap.yaml +++ b/roadmap.yaml @@ -449,8 +449,9 @@ epics: depends_on: [] - id: release-qa-gate-playwright title: "T2: wire the Playwright Web UI sweep into the gate (currently manual-trigger only)" - status: todo + status: in_review priority: P2 + note: "Landed ADVISORY, not blocking (FR-016 wants blocking eventually). The sweep itself is now committed (e2e/web-ui-sweep: servers list, server detail + security tab, tools page + search, activity log, settings; uncaught page exceptions fail it) and scripts/run-web-smoke.sh — previously dead, it pointed at a spec file that lives in a gitignored dir — is its single launcher for both `./scripts/run-web-smoke.sh` by hand and the gate's `web-ui-sweep` job. Advisory is enforced twice: continue-on-error on the job (a failing job inside a reusable workflow would otherwise become the workflow_call conclusion the publishers gate on) and a non-blocking manifest entry advisory/web-ui-sweep (renamed from reserved/web-ui-sweep) whose failures land in advisory_failures via the new `release-gate run-suite --advisory` flag. Promotion to blocking = Blocking:true + drop continue-on-error, after 3 consecutive clean tags." depends_on: [release-qa-gate-matrix] - id: release-qa-gate-macos title: "T3: macOS app smoke on a macos runner, advisory until 3 consecutive passes (today zero CI automation for the tray app)" diff --git a/scripts/run-web-smoke.sh b/scripts/run-web-smoke.sh index 82337317d..509cdfee0 100755 --- a/scripts/run-web-smoke.sh +++ b/scripts/run-web-smoke.sh @@ -1,4 +1,14 @@ #!/usr/bin/env bash +# +# Boots a throwaway mcpproxy instance and runs the Playwright Web UI sweep +# (e2e/web-ui-sweep) against the Web UI IT serves — embedded frontend, never a +# dev server. See docs/development/web-ui-verification.md. +# +# Same script both ways it is used: +# * by hand: ./scripts/run-web-smoke.sh --show-report +# * by the release gate: .github/workflows/release-qa-gate.yml `web-ui-sweep` +# job (advisory), with MCPPROXY_BINARY_PATH / MCPPROXY_FIXTURE_PATH pointing +# at the candidate binaries it downloaded. set -euo pipefail @@ -10,11 +20,19 @@ usage() { cat </tmp/web-smoke-artifacts) EOF } @@ -45,58 +63,86 @@ required() { fi } -required go required curl required node -required npx +required npm SCRIPT_DIR=$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd) REPO_ROOT=$(cd "$SCRIPT_DIR/.." && pwd) BINARY_PATH="${MCPPROXY_BINARY_PATH:-$REPO_ROOT/mcpproxy}" +FIXTURE_PATH="${MCPPROXY_FIXTURE_PATH:-}" BASE_URL="${MCPPROXY_BASE_URL:-http://127.0.0.1:18080}" -PLAYWRIGHT_WORKDIR="$REPO_ROOT/.playwright-mcp" -RESULTS_DIR="$PLAYWRIGHT_WORKDIR/test-results" +LISTEN="${BASE_URL#http://}" +SWEEP_DIR="$REPO_ROOT/e2e/web-ui-sweep" ARTIFACT_DIR="${ARTIFACT_DIR:-$REPO_ROOT/tmp/web-smoke-artifacts}" +REPORT_DIR="$ARTIFACT_DIR/playwright-report" +API_KEY="${MCPPROXY_API_KEY:-web-sweep-key}" +# The sweep's server-dependent checks run only when a fixture upstream exists. +SWEEP_SERVER_NAME="" -mkdir -p "$ARTIFACT_DIR" -mkdir -p "$PLAYWRIGHT_WORKDIR" +mkdir -p "$ARTIFACT_DIR" "$REPORT_DIR" -pushd "$REPO_ROOT" >/dev/null if [[ ! -x "$BINARY_PATH" ]]; then + required go echo "building mcpproxy binary..." - go build -o "$BINARY_PATH" ./cmd/mcpproxy + (cd "$REPO_ROOT" && go build -o "$BINARY_PATH" ./cmd/mcpproxy) fi -popd >/dev/null -TMPDIR=$(mktemp -d) -CONFIG_PATH="$TMPDIR/config.json" -DATA_DIR="$TMPDIR/data" -LOG_PATH="$TMPDIR/mcpproxy.log" +TMPDIR_SWEEP=$(mktemp -d) +CONFIG_PATH="$TMPDIR_SWEEP/config.json" +DATA_DIR="$TMPDIR_SWEEP/data" +LOG_PATH="$TMPDIR_SWEEP/mcpproxy.log" cleanup() { - if [[ -n "${SERVER_PID:-}" ]]; then - if kill -0 "$SERVER_PID" >/dev/null 2>&1; then - kill "$SERVER_PID" >/dev/null 2>&1 || true - wait "$SERVER_PID" >/dev/null 2>&1 || true - fi + if [[ -n "${SERVER_PID:-}" ]] && kill -0 "$SERVER_PID" >/dev/null 2>&1; then + kill "$SERVER_PID" >/dev/null 2>&1 || true + wait "$SERVER_PID" >/dev/null 2>&1 || true fi - rm -rf "$TMPDIR" + rm -rf "$TMPDIR_SWEEP" } trap cleanup EXIT +# mcpServers: the stdio fixture when one was supplied, otherwise an empty fleet +# (the sweep then skips its server-dependent checks instead of failing). +SERVERS_JSON="[]" +if [[ -n "$FIXTURE_PATH" ]]; then + if [[ ! -x "$FIXTURE_PATH" ]]; then + echo "MCPPROXY_FIXTURE_PATH is not executable: $FIXTURE_PATH" >&2 + exit 1 + fi + SWEEP_SERVER_NAME="sweep-stdio" + SERVERS_JSON=$(cat <"$CONFIG_PATH" { - "listen": "127.0.0.1:18080", + "listen": "${LISTEN}", "data_dir": "${DATA_DIR}", - "api_key": "", + "api_key": "${API_KEY}", "enable_tray": false, + "enable_socket": false, + "enable_web_ui": true, + "check_server_repo": false, "logging": { "level": "info", "enable_file": false, "enable_console": true }, - "mcpServers": [], + "mcpServers": ${SERVERS_JSON}, "top_k": 10, "tools_limit": 20, "tool_response_limit": 20000, @@ -110,17 +156,21 @@ cat <"$CONFIG_PATH" } JSON -"$BINARY_PATH" serve --config "$CONFIG_PATH" --listen 127.0.0.1:18080 >"$LOG_PATH" 2>&1 & +# HEADLESS/DO_NOT_TRACK: a QA sweep must never open a browser for OAuth nor +# emit production telemetry (same rule the gate driver applies). +HEADLESS=1 DO_NOT_TRACK=1 "$BINARY_PATH" serve \ + --config "$CONFIG_PATH" --listen "$LISTEN" >"$LOG_PATH" 2>&1 & SERVER_PID=$! -echo "mcpproxy started (PID ${SERVER_PID}); waiting for readiness..." +echo "mcpproxy started (PID ${SERVER_PID}); waiting for readiness at ${BASE_URL}..." attempt=0 -until curl -sS -o /dev/null -w '%{http_code}' "$BASE_URL/api/v1/servers" | grep -q '^200$'; do +until curl -sS -o /dev/null -w '%{http_code}' \ + -H "X-API-Key: ${API_KEY}" "$BASE_URL/api/v1/servers" | grep -q '^200$'; do sleep 1 attempt=$((attempt + 1)) if [[ $attempt -gt 45 ]]; then - echo "server did not become ready after ${attempt}s" + echo "server did not become ready after ${attempt}s" >&2 cat "$LOG_PATH" >&2 exit 1 fi @@ -128,33 +178,22 @@ done echo "server ready at $BASE_URL" -rm -rf "$RESULTS_DIR" - -export MCPPROXY_BASE_URL="$BASE_URL" -export PLAYWRIGHT_HTML_PATH="$ARTIFACT_DIR/playwright-report" export PLAYWRIGHT_BROWSERS_PATH="${PLAYWRIGHT_BROWSERS_PATH:-$REPO_ROOT/tmp/playwright-browsers}" -export CI=${CI:-1} - -mkdir -p "$PLAYWRIGHT_HTML_PATH" +mkdir -p "$PLAYWRIGHT_BROWSERS_PATH" -if [[ ! -d "$PLAYWRIGHT_WORKDIR/node_modules/@playwright/test" ]]; then - echo "installing @playwright/test into $PLAYWRIGHT_WORKDIR" - npm install --prefix "$PLAYWRIGHT_WORKDIR" --no-save --package-lock=false @playwright/test +if [[ ! -x "$SWEEP_DIR/node_modules/.bin/playwright" ]]; then + # Installs the @playwright/test range from e2e/web-ui-sweep/package.json. + echo "installing @playwright/test into $SWEEP_DIR" + npm install --prefix "$SWEEP_DIR" --no-save --package-lock=false fi -export PATH="$PLAYWRIGHT_WORKDIR/node_modules/.bin:$PATH" - -PLAYWRIGHT_BIN="$PLAYWRIGHT_WORKDIR/node_modules/.bin/playwright" +PLAYWRIGHT_BIN="$SWEEP_DIR/node_modules/.bin/playwright" if [[ ! -x "$PLAYWRIGHT_BIN" ]]; then echo "playwright CLI not found after install" >&2 exit 1 fi -mkdir -p "$PLAYWRIGHT_BROWSERS_PATH" - -pushd "$PLAYWRIGHT_WORKDIR" >/dev/null - -echo "installing Playwright browsers (cached under $PLAYWRIGHT_BROWSERS_PATH)" +echo "installing Playwright Chromium (cached under $PLAYWRIGHT_BROWSERS_PATH)" if [[ "$(uname -s)" == "Linux" ]]; then "$PLAYWRIGHT_BIN" install --with-deps chromium else @@ -162,29 +201,29 @@ else fi set +e -"$PLAYWRIGHT_BIN" test web-smoke.spec.ts --project=chromium +( + cd "$SWEEP_DIR" || exit 1 + MCPPROXY_BASE_URL="$BASE_URL" \ + MCPPROXY_API_KEY="$API_KEY" \ + SWEEP_SERVER_NAME="$SWEEP_SERVER_NAME" \ + SWEEP_REPORT_DIR="$REPORT_DIR" \ + "$PLAYWRIGHT_BIN" test web-ui-sweep.spec.ts +) PLAYWRIGHT_STATUS=$? set -e -popd >/dev/null - cp "$LOG_PATH" "$ARTIFACT_DIR/server.log" -if [[ -d "$RESULTS_DIR" ]]; then - mkdir -p "$ARTIFACT_DIR/test-results" - cp -R "$RESULTS_DIR/." "$ARTIFACT_DIR/test-results/" >/dev/null 2>&1 || true -fi - if [[ $PLAYWRIGHT_STATUS -ne 0 ]]; then - echo "web smoke failed; artifacts stored in $ARTIFACT_DIR" >&2 + echo "web UI sweep FAILED; artifacts (HTML report + server log) in $ARTIFACT_DIR" >&2 exit $PLAYWRIGHT_STATUS fi -echo "web smoke passed; artifacts stored in $ARTIFACT_DIR" +echo "web UI sweep passed; artifacts stored in $ARTIFACT_DIR" if [[ $SHOW_REPORT -eq 1 ]]; then echo "launching Playwright HTML report (Ctrl+C to exit)..." - "$PLAYWRIGHT_BIN" show-report "$PLAYWRIGHT_HTML_PATH" + "$PLAYWRIGHT_BIN" show-report "$REPORT_DIR" fi exit 0 From 352cfd988a3339225c9ce0f76c11f8b9b7fe099b Mon Sep 17 00:00:00 2001 From: Algis Dumbris Date: Sun, 23 Aug 2026 22:00:23 +0300 Subject: [PATCH 2/5] fix(ci): keep advisory sweep artifacts out of release assets; harden launcher MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Cross-model review follow-ups on the T2 Playwright sweep. The gate runs as a reusable workflow, so its artifacts land in the publisher's own run — and release.yml/prerelease.yml build release assets by globbing every *.tar.gz/*.zip from ALL run artifacts. A Playwright HTML report keeps its traces as playwright-report/data/ .zip, retained on failure (and on a flaky-then-green retry), so a red advisory sweep on a tag would have attached unnamed trace zips to the public release. Both publishers now exclude the sweep artifact from that glob, guarded by TestPublishersDoNotShipSweepArtifacts. scripts/run-web-smoke.sh: - generate a throwaway API key instead of honouring MCPPROXY_API_KEY, which the sweep writes into every navigation URL and Playwright then stores in the HTML report and failure traces; pin it into the server's environment too, since the env var outranks the config file and an ambient key otherwise left the server demanding one key while the probe used another (46s of 401s, then a readiness timeout) - fail fast when the candidate exits before becoming ready instead of burning the full timeout, and give the readiness curl connect/max timeouts so a port held by a non-responding process cannot block the loop forever (occupied-port repro: 119s -> 5s) - preserve the server log from the cleanup trap so an early failure is diagnosable from the uploaded artifact, not just the CI console - reject fixture paths containing a quote/backslash/newline rather than silently emitting a malformed config heredoc - npm ci against a committed package-lock.json instead of resolving the mutable ^1.49.0 range during release qualification --- .github/workflows/prerelease.yml | 7 +- .github/workflows/release.yml | 10 ++- cmd/release-gate/webui_sweep_audit_test.go | 42 ++++++++++++ docs/development/release-gate.md | 11 +++ docs/development/web-ui-verification.md | 2 +- e2e/web-ui-sweep/package-lock.json | 78 ++++++++++++++++++++++ scripts/run-web-smoke.sh | 58 ++++++++++++++-- 7 files changed, 197 insertions(+), 11 deletions(-) create mode 100644 e2e/web-ui-sweep/package-lock.json diff --git a/.github/workflows/prerelease.yml b/.github/workflows/prerelease.yml index 4c97daf7e..616712778 100644 --- a/.github/workflows/prerelease.yml +++ b/.github/workflows/prerelease.yml @@ -888,8 +888,11 @@ jobs: # Create a flat structure to avoid duplicates mkdir -p release-files - # Copy archives (tar.gz and zip files) - only versioned, no latest for prereleases - find dist -name "*.tar.gz" -o -name "*.zip" | while read file; do + # Copy archives (tar.gz and zip files) - only versioned, no latest for prereleases. + # The QA gate runs as a reusable workflow of THIS run, so its artifacts land + # in dist/ too; its Playwright HTML report stores traces as + # playwright-report/data/.zip and must never become a release asset. + find dist -not -path "*/web-ui-sweep-playwright-report/*" \( -name "*.tar.gz" -o -name "*.zip" \) | while read file; do filename=$(basename "$file") cp "$file" "release-files/$filename" done diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index 98ce9ca12..45ea95d87 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -1298,8 +1298,14 @@ jobs: # Create a flat structure to avoid duplicates mkdir -p release-files - # Copy archives (tar.gz and zip files) - find dist -name "*.tar.gz" -o -name "*.zip" | while read file; do + # Copy archives (tar.gz and zip files). + # The QA gate runs as a reusable workflow of THIS run, so its artifacts + # land in dist/ too. Its Playwright HTML report stores traces as + # playwright-report/data/.zip (retained on failure — precisely the + # case the advisory sweep tolerates), which would otherwise be published + # as unnamed release assets. Excluded here, not upstream, so the report + # stays downloadable as a CI artifact for post-mortems. + find dist -not -path "*/web-ui-sweep-playwright-report/*" \( -name "*.tar.gz" -o -name "*.zip" \) | while read file; do filename=$(basename "$file") cp "$file" "release-files/$filename" done diff --git a/cmd/release-gate/webui_sweep_audit_test.go b/cmd/release-gate/webui_sweep_audit_test.go index 941b52035..3844b97f4 100644 --- a/cmd/release-gate/webui_sweep_audit_test.go +++ b/cmd/release-gate/webui_sweep_audit_test.go @@ -102,3 +102,45 @@ func yamlFlatten(n *yaml.Node) string { walk(n) return b.String() } + +// sweepArtifactName is the advisory job's Playwright artifact. It lands in the +// SAME workflow run as the publishers (the gate is a reusable workflow the +// publishers call via `uses:`), so anything the publishers glob out of "all +// artifacts" can pick it up. +const sweepArtifactName = "web-ui-sweep-playwright-report" + +// TestPublishersDoNotShipSweepArtifacts guards a sharp edge introduced by +// wiring the sweep into the gate: release.yml / prerelease.yml download EVERY +// artifact of the run and copy every *.tar.gz / *.zip they find into the +// published release assets. A Playwright HTML report contains its traces as +// `playwright-report/data/.zip`, and traces are retained on failure — +// exactly the case the advisory job is designed to tolerate. Without an +// exclusion, a red (or merely flaky-then-green) sweep on a tag attaches +// unnamed trace zips to the public release. +func TestPublishersDoNotShipSweepArtifacts(t *testing.T) { + for _, name := range []string{"release.yml", "prerelease.yml"} { + t.Run(name, func(t *testing.T) { + path := filepath.Join("..", "..", ".github", "workflows", name) + wf := parseWorkflow(t, path) + + var checked int + for jobName, job := range wf.Jobs { + for _, s := range job.Steps { + // The indiscriminate archive collector: `find dist -name "*.zip" ...` + if !strings.Contains(s.Run, `-name "*.zip"`) { + continue + } + checked++ + if !strings.Contains(s.Run, sweepArtifactName) { + t.Errorf("%s job %q collects every *.zip into the release assets without excluding %q; "+ + "a failed advisory sweep would publish Playwright trace zips as release files:\n%s", + name, jobName, sweepArtifactName, s.Run) + } + } + } + if checked == 0 { + t.Fatalf("%s: found no archive-collection step to audit (did the publisher change shape?)", name) + } + }) + } +} diff --git a/docs/development/release-gate.md b/docs/development/release-gate.md index c82533d65..7773fbcb7 100644 --- a/docs/development/release-gate.md +++ b/docs/development/release-gate.md @@ -129,6 +129,17 @@ It is **advisory**, on purpose while the sweep earns its flake record: - the Playwright HTML report + server log are uploaded as the `web-ui-sweep-playwright-report` artifact (14 days) for post-mortem. +> **Do not rename that artifact casually.** The gate is a *reusable* workflow, so +> its artifacts land in the publisher's own run, and `release.yml` / +> `prerelease.yml` build their release assets by globbing every `*.tar.gz` / +> `*.zip` out of *all* run artifacts. A Playwright report stores its traces as +> `playwright-report/data/.zip` (retained on failure — exactly what an +> advisory sweep tolerates), so both publishers exclude +> `*/web-ui-sweep-playwright-report/*` from that glob by name. Renaming the +> artifact without updating both publishers would publish unnamed trace zips as +> public release assets; +> `TestPublishersDoNotShipSweepArtifacts` guards this. + **Promotion to blocking** is a two-line change: set `Blocking: true` on the `advisory/web-ui-sweep` manifest entry and drop `continue-on-error` from the job (the FR-016 end state). Do it once the sweep has passed on three consecutive diff --git a/docs/development/web-ui-verification.md b/docs/development/web-ui-verification.md index 20cb463c1..1925e5e9b 100644 --- a/docs/development/web-ui-verification.md +++ b/docs/development/web-ui-verification.md @@ -16,7 +16,7 @@ The core-screen sweep is committed and scripted — run it before you hand-roll ./scripts/run-web-smoke.sh --show-report # boots a throwaway instance, runs e2e/web-ui-sweep ``` -The launcher builds `./mcpproxy` if needed, serves a throwaway instance on `127.0.0.1:18080`, installs Chromium, and runs [`e2e/web-ui-sweep/web-ui-sweep.spec.ts`](https://github.com/smart-mcp-proxy/mcpproxy-go/blob/main/e2e/web-ui-sweep/web-ui-sweep.spec.ts) — servers list, server detail (+ security tab), tools page and search, activity log, settings — failing on uncaught page exceptions. Pass `MCPPROXY_FIXTURE_PATH=$(go build -o /tmp/mcpfixture ./cmd/mcpfixture && echo /tmp/mcpfixture)` to register a live stdio upstream so the server- and tool-dependent checks run instead of skipping. The HTML report lands in `tmp/web-smoke-artifacts/playwright-report/`. +The launcher builds `./mcpproxy` if needed, serves a throwaway instance on `127.0.0.1:18080` under a freshly generated throwaway API key (your own `MCPPROXY_API_KEY` is deliberately ignored — the key ends up in report URLs and traces), installs Chromium from the committed `e2e/web-ui-sweep/package-lock.json` via `npm ci`, and runs [`e2e/web-ui-sweep/web-ui-sweep.spec.ts`](https://github.com/smart-mcp-proxy/mcpproxy-go/blob/main/e2e/web-ui-sweep/web-ui-sweep.spec.ts) — servers list, server detail (+ security tab), tools page and search, activity log, settings — failing on uncaught page exceptions. Pass `MCPPROXY_FIXTURE_PATH=$(go build -o /tmp/mcpfixture ./cmd/mcpfixture && echo /tmp/mcpfixture)` to register a live stdio upstream so the server- and tool-dependent checks run instead of skipping. The HTML report lands in `tmp/web-smoke-artifacts/playwright-report/`. The release QA gate runs this exact script on every tag as its **advisory** `web-ui-sweep` job — see [Release Gate](release-gate.md#web-ui-sweep-t2--advisory). Extend the committed sweep when you add a screen worth guarding on releases; use the ad-hoc pattern below for the deeper, spec-specific verification that ships beside a spec. diff --git a/e2e/web-ui-sweep/package-lock.json b/e2e/web-ui-sweep/package-lock.json new file mode 100644 index 000000000..c2a1299fe --- /dev/null +++ b/e2e/web-ui-sweep/package-lock.json @@ -0,0 +1,78 @@ +{ + "name": "mcpproxy-web-ui-sweep", + "version": "1.0.0", + "lockfileVersion": 3, + "requires": true, + "packages": { + "": { + "name": "mcpproxy-web-ui-sweep", + "version": "1.0.0", + "devDependencies": { + "@playwright/test": "^1.49.0" + } + }, + "node_modules/@playwright/test": { + "version": "1.62.1", + "resolved": "https://registry.npmjs.org/@playwright/test/-/test-1.62.1.tgz", + "integrity": "sha512-DTcUc8qii+cpHvtOwggMtBRMjKZHXYWdw8syRYu2vtzuq4Wxphqq4NfCs5Zt44L6mA8rfDfj+PHnxFc/FeK6mQ==", + "dev": true, + "license": "Apache-2.0", + "dependencies": { + "playwright": "1.62.1" + }, + "bin": { + "playwright": "cli.js" + }, + "engines": { + "node": ">=20" + } + }, + "node_modules/fsevents": { + "version": "2.3.2", + "resolved": "https://registry.npmjs.org/fsevents/-/fsevents-2.3.2.tgz", + "integrity": "sha512-xiqMQR4xAeHTuB9uWm+fFRcIOgKBMiOBP+eXiyT7jsgVCq1bkVygt00oASowB7EdtpOHaaPgKt812P9ab+DDKA==", + "dev": true, + "hasInstallScript": true, + "license": "MIT", + "optional": true, + "os": [ + "darwin" + ], + "engines": { + "node": "^8.16.0 || ^10.6.0 || >=11.0.0" + } + }, + "node_modules/playwright": { + "version": "1.62.1", + "resolved": "https://registry.npmjs.org/playwright/-/playwright-1.62.1.tgz", + "integrity": "sha512-0M+L3LAD8/nm554LOla9Ayx0j0tmFZ0FBcoQ7F1VuVHpM/XpiC8RcDzBQB8W5+hA8L22THxELzeF+2WcUzvcLg==", + "dev": true, + "license": "Apache-2.0", + "dependencies": { + "playwright-core": "1.62.1" + }, + "bin": { + "playwright": "cli.js" + }, + "engines": { + "node": ">=20" + }, + "optionalDependencies": { + "fsevents": "2.3.2" + } + }, + "node_modules/playwright-core": { + "version": "1.62.1", + "resolved": "https://registry.npmjs.org/playwright-core/-/playwright-core-1.62.1.tgz", + "integrity": "sha512-wPYSwEBJY9GHraISXqyqtx0na0LpO3XEX7jNDhntbex7tzUS7kLnZsOlFruFJB4Hi/rhDMjXGqHewDZ68nYZVw==", + "dev": true, + "license": "Apache-2.0", + "bin": { + "playwright-core": "cli.js" + }, + "engines": { + "node": ">=20" + } + } + } +} diff --git a/scripts/run-web-smoke.sh b/scripts/run-web-smoke.sh index 509cdfee0..f44a59e73 100755 --- a/scripts/run-web-smoke.sh +++ b/scripts/run-web-smoke.sh @@ -77,7 +77,12 @@ LISTEN="${BASE_URL#http://}" SWEEP_DIR="$REPO_ROOT/e2e/web-ui-sweep" ARTIFACT_DIR="${ARTIFACT_DIR:-$REPO_ROOT/tmp/web-smoke-artifacts}" REPORT_DIR="$ARTIFACT_DIR/playwright-report" -API_KEY="${MCPPROXY_API_KEY:-web-sweep-key}" +# Always a fresh throwaway key, NEVER $MCPPROXY_API_KEY: the sweep puts the key +# in every navigation URL, and Playwright stores those URLs in the HTML report +# and failure traces. Honouring an ambient MCPPROXY_API_KEY would copy a +# developer's real key into tmp/web-smoke-artifacts (and, in CI, an uploaded +# artifact). The instance is throwaway, so its key may as well be too. +API_KEY="web-sweep-$(date +%s)-$$" # The sweep's server-dependent checks run only when a fixture upstream exists. SWEEP_SERVER_NAME="" @@ -99,6 +104,13 @@ cleanup() { kill "$SERVER_PID" >/dev/null 2>&1 || true wait "$SERVER_PID" >/dev/null 2>&1 || true fi + # Preserve the server log before the temp dir goes away. Doing this in the + # trap (not only on the happy path) is what makes an EARLY failure — + # readiness timeout, npm/Chromium install blowing up under `set -e` — + # diagnosable from the uploaded artifact instead of only from the CI console. + if [[ -f "$LOG_PATH" ]]; then + cp "$LOG_PATH" "$ARTIFACT_DIR/server.log" 2>/dev/null || true + fi rm -rf "$TMPDIR_SWEEP" } trap cleanup EXIT @@ -111,6 +123,16 @@ if [[ -n "$FIXTURE_PATH" ]]; then echo "MCPPROXY_FIXTURE_PATH is not executable: $FIXTURE_PATH" >&2 exit 1 fi + # The config below is a heredoc, not JSON-encoded output: a path containing a + # quote, a backslash or a newline would silently produce a malformed (or + # subtly different) config rather than an error. Reject it loudly instead of + # taking on a jq/python dependency for a path that is normally boring. + case "$FIXTURE_PATH" in + *'"'* | *\\* | *$'\n'*) + echo "MCPPROXY_FIXTURE_PATH contains a quote, backslash or newline and cannot be embedded in the sweep config: $FIXTURE_PATH" >&2 + exit 1 + ;; + esac SWEEP_SERVER_NAME="sweep-stdio" SERVERS_JSON=$(cat <"$LOG_PATH" 2>&1 & SERVER_PID=$! echo "mcpproxy started (PID ${SERVER_PID}); waiting for readiness at ${BASE_URL}..." attempt=0 -until curl -sS -o /dev/null -w '%{http_code}' \ +# --connect-timeout/--max-time are load-bearing, not decoration: a process that +# holds the port but never answers (a hung instance, a half-open socket) makes an +# untimed curl block forever, and the liveness check below would never get a turn +# — the job would sit until its 20-minute timeout instead of failing in seconds. +until curl -s --connect-timeout 2 --max-time 5 -o /dev/null -w '%{http_code}' \ -H "X-API-Key: ${API_KEY}" "$BASE_URL/api/v1/servers" | grep -q '^200$'; do + # Fail fast if the candidate already exited (port in use → exit code 2, bad + # config → 4, DB locked → 3). Without this the loop would burn the full + # timeout, and — worse — a leftover instance of a PREVIOUS sweep listening on + # the same port could answer 200 and the sweep would silently exercise that + # stale binary instead of the candidate. + if ! kill -0 "$SERVER_PID" >/dev/null 2>&1; then + wait "$SERVER_PID" >/dev/null 2>&1 || true + echo "mcpproxy exited before becoming ready at ${BASE_URL} (is ${LISTEN} already in use?)" >&2 + cat "$LOG_PATH" >&2 + exit 1 + fi sleep 1 attempt=$((attempt + 1)) if [[ $attempt -gt 45 ]]; then @@ -182,9 +224,13 @@ export PLAYWRIGHT_BROWSERS_PATH="${PLAYWRIGHT_BROWSERS_PATH:-$REPO_ROOT/tmp/play mkdir -p "$PLAYWRIGHT_BROWSERS_PATH" if [[ ! -x "$SWEEP_DIR/node_modules/.bin/playwright" ]]; then - # Installs the @playwright/test range from e2e/web-ui-sweep/package.json. - echo "installing @playwright/test into $SWEEP_DIR" - npm install --prefix "$SWEEP_DIR" --no-save --package-lock=false + # `npm ci` against the COMMITTED e2e/web-ui-sweep/package-lock.json, never a + # bare `npm install`: this runs during release qualification, and resolving the + # mutable `^1.49.0` range there would execute whatever Playwright and its + # transitive deps published since the last review. The lockfile pins exact + # versions + integrity hashes; bump it deliberately with `npm install`. + echo "installing @playwright/test into $SWEEP_DIR (npm ci, locked)" + npm ci --prefix "$SWEEP_DIR" fi PLAYWRIGHT_BIN="$SWEEP_DIR/node_modules/.bin/playwright" From 1a791e8a72d131707256344ce44e14523aad08b6 Mon Sep 17 00:00:00 2001 From: Algis Dumbris Date: Sun, 23 Aug 2026 22:07:47 +0300 Subject: [PATCH 3/5] fix(ci): enforce the sweep lockfile against stale node_modules MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round-2 review follow-up. `npm ci` ran only when the playwright binary was missing, so a tree installed by an older revision of this script (unlocked `npm install`) kept serving an unreviewed version forever — the lockfile was enforced on fresh CI runners but not in existing worktrees. Compare the installed @playwright/test version against the lockfile instead of merely checking for presence: a mismatch reinstalls, a match still skips. That closes the hole without making every hand-run pay a registry round-trip, which running `npm ci` unconditionally would. --- scripts/run-web-smoke.sh | 41 ++++++++++++++++++++++++++++++++-------- 1 file changed, 33 insertions(+), 8 deletions(-) diff --git a/scripts/run-web-smoke.sh b/scripts/run-web-smoke.sh index f44a59e73..4abf85652 100755 --- a/scripts/run-web-smoke.sh +++ b/scripts/run-web-smoke.sh @@ -223,14 +223,39 @@ echo "server ready at $BASE_URL" export PLAYWRIGHT_BROWSERS_PATH="${PLAYWRIGHT_BROWSERS_PATH:-$REPO_ROOT/tmp/playwright-browsers}" mkdir -p "$PLAYWRIGHT_BROWSERS_PATH" -if [[ ! -x "$SWEEP_DIR/node_modules/.bin/playwright" ]]; then - # `npm ci` against the COMMITTED e2e/web-ui-sweep/package-lock.json, never a - # bare `npm install`: this runs during release qualification, and resolving the - # mutable `^1.49.0` range there would execute whatever Playwright and its - # transitive deps published since the last review. The lockfile pins exact - # versions + integrity hashes; bump it deliberately with `npm install`. - echo "installing @playwright/test into $SWEEP_DIR (npm ci, locked)" - npm ci --prefix "$SWEEP_DIR" +# `npm ci` against the COMMITTED e2e/web-ui-sweep/package-lock.json, never a +# bare `npm install`: this runs during release qualification, and resolving the +# mutable `^1.49.0` range there would execute whatever Playwright and its +# transitive deps published since the last review. The lockfile pins exact +# versions + integrity hashes; bump it deliberately with `npm install`. +# +# The install is skipped only when what is on disk ALREADY MATCHES the lockfile. +# A mere "is playwright present?" check would let a stale tree — e.g. one an +# older revision of this script installed with an unlocked `npm install` — keep +# serving an unreviewed version forever. Comparing versions keeps the hand-run +# fast path (no registry round-trip when nothing changed) without weakening that. +locked_playwright_version() { + node -p "require('$SWEEP_DIR/package-lock.json').packages['node_modules/@playwright/test'].version" 2>/dev/null || true +} +installed_playwright_version() { + node -p "require('$SWEEP_DIR/node_modules/@playwright/test/package.json').version" 2>/dev/null || true +} + +LOCKED_PW=$(locked_playwright_version) +if [[ -z "$LOCKED_PW" ]]; then + echo "cannot read the pinned @playwright/test version from $SWEEP_DIR/package-lock.json" >&2 + exit 1 +fi + +if [[ ! -x "$SWEEP_DIR/node_modules/.bin/playwright" || "$(installed_playwright_version)" != "$LOCKED_PW" ]]; then + echo "installing @playwright/test@${LOCKED_PW} into $SWEEP_DIR (npm ci, locked)" + # cd rather than `npm ci --prefix`: --prefix has a long history of version- + # dependent behaviour for `ci` specifically, and this runs during release + # qualification — an install that silently resolves against the repo root + # instead of the sweep's lockfile is not a failure mode worth risking. + (cd "$SWEEP_DIR" && npm ci) +else + echo "@playwright/test@${LOCKED_PW} already installed (matches lockfile)" fi PLAYWRIGHT_BIN="$SWEEP_DIR/node_modules/.bin/playwright" From 3e21218fe392f49d96263cb710276d47419a7455 Mon Sep 17 00:00:00 2001 From: Algis Dumbris Date: Sun, 23 Aug 2026 22:21:02 +0300 Subject: [PATCH 4/5] fix(ci): stamp the sweep install with the lockfile hash Round-3 review follow-up, closing two low-severity residuals in the install guard. Comparing only @playwright/test's version still missed a lockfile that moved a transitive dependency (playwright-core) without touching the top-level version. Hash the whole lockfile instead and stamp it into node_modules after a successful npm ci; any drift reinstalls. Verified against a transitive-only lockfile bump, which the version check would have skipped. The stamp is written only on success, so a failed npm ci leaves the previous, still-accurate stamp in place. Also pass the path through process.argv rather than interpolating it into the node -e source: a checkout under a path containing a quote produced invalid JavaScript and a silently empty result. --- scripts/run-web-smoke.sh | 44 +++++++++++++++++++++++++--------------- 1 file changed, 28 insertions(+), 16 deletions(-) diff --git a/scripts/run-web-smoke.sh b/scripts/run-web-smoke.sh index 4abf85652..c949c9fa1 100755 --- a/scripts/run-web-smoke.sh +++ b/scripts/run-web-smoke.sh @@ -229,33 +229,45 @@ mkdir -p "$PLAYWRIGHT_BROWSERS_PATH" # transitive deps published since the last review. The lockfile pins exact # versions + integrity hashes; bump it deliberately with `npm install`. # -# The install is skipped only when what is on disk ALREADY MATCHES the lockfile. -# A mere "is playwright present?" check would let a stale tree — e.g. one an -# older revision of this script installed with an unlocked `npm install` — keep -# serving an unreviewed version forever. Comparing versions keeps the hand-run -# fast path (no registry round-trip when nothing changed) without weakening that. -locked_playwright_version() { - node -p "require('$SWEEP_DIR/package-lock.json').packages['node_modules/@playwright/test'].version" 2>/dev/null || true -} -installed_playwright_version() { - node -p "require('$SWEEP_DIR/node_modules/@playwright/test/package.json').version" 2>/dev/null || true +# The install is skipped only when what is on disk was installed FROM THE CURRENT +# lockfile. A mere "is playwright present?" check would let a stale tree — e.g. +# one an older revision of this script installed with an unlocked `npm install` — +# keep serving an unreviewed version forever; checking only @playwright/test's +# version would still miss a lockfile that moved a transitive dependency. So the +# hash of the whole lockfile is stamped into node_modules after a successful +# `npm ci`, and any drift re-runs it. This keeps the hand-run fast path (no +# registry round-trip when nothing changed) without weakening the pinning. +# +# The path goes through process.argv, never string interpolation into the JS — +# a repo checked out under a path containing a quote would otherwise produce +# invalid JavaScript and a silently empty result. +lock_hash() { + node -e 'const c=require("crypto"),f=require("fs");process.stdout.write(c.createHash("sha256").update(f.readFileSync(process.argv[1])).digest("hex"))' "$1" 2>/dev/null || true } -LOCKED_PW=$(locked_playwright_version) -if [[ -z "$LOCKED_PW" ]]; then - echo "cannot read the pinned @playwright/test version from $SWEEP_DIR/package-lock.json" >&2 +LOCK_STAMP="$SWEEP_DIR/node_modules/.sweep-lock-sha256" +WANT_LOCK_HASH=$(lock_hash "$SWEEP_DIR/package-lock.json") +if [[ -z "$WANT_LOCK_HASH" ]]; then + echo "cannot hash $SWEEP_DIR/package-lock.json — is the lockfile committed?" >&2 exit 1 fi -if [[ ! -x "$SWEEP_DIR/node_modules/.bin/playwright" || "$(installed_playwright_version)" != "$LOCKED_PW" ]]; then - echo "installing @playwright/test@${LOCKED_PW} into $SWEEP_DIR (npm ci, locked)" +HAVE_LOCK_HASH="" +if [[ -f "$LOCK_STAMP" ]]; then + HAVE_LOCK_HASH=$(cat "$LOCK_STAMP") +fi + +if [[ ! -x "$SWEEP_DIR/node_modules/.bin/playwright" || "$HAVE_LOCK_HASH" != "$WANT_LOCK_HASH" ]]; then + echo "installing the locked @playwright/test into $SWEEP_DIR (npm ci)" # cd rather than `npm ci --prefix`: --prefix has a long history of version- # dependent behaviour for `ci` specifically, and this runs during release # qualification — an install that silently resolves against the repo root # instead of the sweep's lockfile is not a failure mode worth risking. (cd "$SWEEP_DIR" && npm ci) + # After npm ci, which wipes node_modules (and with it any previous stamp). + printf '%s' "$WANT_LOCK_HASH" >"$LOCK_STAMP" else - echo "@playwright/test@${LOCKED_PW} already installed (matches lockfile)" + echo "@playwright/test already installed from the current lockfile" fi PLAYWRIGHT_BIN="$SWEEP_DIR/node_modules/.bin/playwright" From 4d35f00af2da69269a63dab5cbea2a27e18287e3 Mon Sep 17 00:00:00 2001 From: Algis Dumbris Date: Mon, 24 Aug 2026 06:41:39 +0300 Subject: [PATCH 5/5] fix(ci): unguessable sweep key; make the tools-search check non-vacuous MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Cross-model review (opencode/gpt-5.6-sol) findings on the advisory Web UI sweep, both verified against the running sweep before fixing: * The throwaway instance's API key was `web-sweep--`. It guards the full REST admin API, which can register a stdio upstream — i.e. run an arbitrary command as whoever launched the sweep — and 127.0.0.1 is reachable by every local account, so a clock+PID key is brute-forceable for the minute the instance is up. Now 24 bytes from crypto.randomBytes. * The tools-search assertion (`count <= before` plus first-row-matches) passed vacuously when the search box did nothing: the unfiltered set is trivially <= itself and the fixture's first tool is already `echo`. It now requires every surviving row to match, which is also race-free — a strict `<` would have raced the background indexer, since `before` can be sampled mid-render. Verified non-vacuous by mutation: with the query emptied the check fails, where the old one passed. A third finding (readiness loop could latch onto a foreign server already on the port) was rejected as a false positive: the probe carries this run's unique key, and a foreign mcpproxy answers 401 — measured — while a second instance on a taken port exits 2 and is caught by the loop's liveness check. --- e2e/web-ui-sweep/web-ui-sweep.spec.ts | 18 +++++++++++++++--- scripts/run-web-smoke.sh | 12 +++++++++++- 2 files changed, 26 insertions(+), 4 deletions(-) diff --git a/e2e/web-ui-sweep/web-ui-sweep.spec.ts b/e2e/web-ui-sweep/web-ui-sweep.spec.ts index 60049b148..40ec2db91 100644 --- a/e2e/web-ui-sweep/web-ui-sweep.spec.ts +++ b/e2e/web-ui-sweep/web-ui-sweep.spec.ts @@ -75,11 +75,23 @@ test('tools page lists upstream tools and search narrows them', async ({ page }) const rows = page.locator('[data-test="tool-row"]') // Tool indexing runs in the background after the upstream connects. await expect.poll(() => rows.count(), { timeout: 30_000 }).toBeGreaterThan(0) - const before = await rows.count() await page.locator('[data-test="tools-search"]').fill('echo') - await expect.poll(() => rows.count(), { timeout: 10_000 }).toBeLessThanOrEqual(before) - await expect(rows.first()).toContainText(/echo/i) + // Assert on the SURVIVORS, not on the row count. "count <= before" passed + // vacuously when the search did nothing at all (the unfiltered set is already + // <= itself, and the first row happened to match), and a strict "<" would race + // the background indexer — `before` can be sampled mid-render. Requiring every + // remaining row to match is race-free and catches a dead search box: the + // fixture's non-matching tool (`ping`) would still be listed. + await expect + .poll( + async () => { + const texts = await rows.allTextContents() + return texts.length > 0 && texts.every((t) => /echo/i.test(t)) + }, + { timeout: 10_000 }, + ) + .toBe(true) expect(errors, `uncaught page errors on /tools: ${errors.join(' | ')}`).toHaveLength(0) }) diff --git a/scripts/run-web-smoke.sh b/scripts/run-web-smoke.sh index c949c9fa1..9fa765973 100755 --- a/scripts/run-web-smoke.sh +++ b/scripts/run-web-smoke.sh @@ -82,7 +82,17 @@ REPORT_DIR="$ARTIFACT_DIR/playwright-report" # and failure traces. Honouring an ambient MCPPROXY_API_KEY would copy a # developer's real key into tmp/web-smoke-artifacts (and, in CI, an uploaded # artifact). The instance is throwaway, so its key may as well be too. -API_KEY="web-sweep-$(date +%s)-$$" +# +# Throwaway, but NOT guessable. The key it protects is the instance's full REST +# admin API, which can register a stdio upstream — i.e. run an arbitrary command +# as whoever launched the sweep. 127.0.0.1 is reachable by every local account, +# so a key derived from the clock and the PID (a search space of seconds x pids) +# is brute-forceable for the minute the sweep is up. 24 random bytes are not. +API_KEY="web-sweep-$(node -e 'process.stdout.write(require("crypto").randomBytes(24).toString("hex"))')" +if [[ ${#API_KEY} -lt 32 ]]; then + echo "failed to generate a random API key for the throwaway instance" >&2 + exit 1 +fi # The sweep's server-dependent checks run only when a fixture upstream exists. SWEEP_SERVER_NAME=""