ci: gate PRs on tests/harness/run-all-replays.sh - #2410
Conversation
Closes the gap between "the harness exists" and "the harness blocks bugs." Phase 2 of the harness roadmap (per tests/harness/README.md): make harness-based E2E a required CI check on every PR touching the tenant binary or the harness itself. Trigger: push + pull_request to staging+main, paths-filtered to workspace-server/**, canvas/**, tests/harness/**, and this workflow. merge_group support included so this becomes branch-protectable. Single-job-with-conditional-steps pattern (matches e2e-api.yml). One check run regardless of paths-filter outcome; satisfies branch protection cleanly per the PR #2264 SKIPPED-in-set finding. Why this exists: 2026-04-30 we shipped a TenantGuard allowlist gap (/buildinfo added to router.go in #2398, never added to the allowlist) that the existing buildinfo-stale-image.sh replay would have caught. The harness was wired correctly; nobody ran it. Replays as a discipline beat replays as a memory item. The CI pipeline: detect-changes (paths filter) └ harness-replays (always) ├ no-op pass when paths-filter says no relevant change └ otherwise: checkout + sibling plugin checkout + /etc/hosts entry + run-all-replays.sh + compose-logs-on-failure + force-teardown Compose logs from tenant/cp-stub/cf-proxy/postgres are dumped on failure so a CI red is debuggable without re-reproducing locally. The trap in run-all-replays.sh handles teardown; the always-run down.sh step is a belt-and-suspenders against trap-bypass kills. Follow-ups (not in this PR): - Add this check to staging branch protection once it's been green for a few PRs (the new-workflow-instability hedge that other gates followed). - Eventually wire the buildx GHA cache to speed up tenant image builds — currently every PR rebuilds the full Dockerfile.tenant (Go + Next.js + template clones) from scratch. Acceptable for now; optimize when the timeout-minutes:30 ceiling becomes painful. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
HongmingWang-Rabbit
left a comment
There was a problem hiding this comment.
Five-axis review (workflow-touching → comment-only per spec):
Correctness ✓ — Single-job-with-per-step-if: pattern is the right shape. The 2026-04-29 corrected memory (feedback_branch_protection_check_name_parity) makes that explicit: two-jobs-sharing-name leaves SKIPPED in the set, which still blocks branch protection. This workflow does the right thing — harness-replays always runs, paths-filter gates the real work.
Readability ✓ — The header comment grounds the gate in #2398's outage cleanly. Inline annotations on /etc/hosts, PLUGIN_REPO_PAT fallback, and trap teardown all earn their lines. Future readers won't have to re-derive why this exists.
Architecture ✓ — Per-SHA concurrency (harness-replays-${{ ... .head.sha || github.sha }}) avoids the auto-promote cancellation deadlock from feedback_concurrency_group_per_sha. cancel-in-progress: false is the correct inverse of the per-ref hazard. merge_group: types: [checks_requested] is present, so the Required workflows have merge_group trigger check is satisfied.
Security ✓ — Both third-party actions SHA-pinned (actions/checkout@34e114876b..., dorny/paths-filter@fbd0ab8f3e...) per feedback#15. secrets.PLUGIN_REPO_PAT || secrets.GITHUB_TOKEN fallback is OK for PR triggers — if molecule-ai-plugin-github-app-auth is private and the PAT is missing, sibling checkout hard-fails (clean), not silent-succeeds. No log-injection surface in the dump-on-failure step.
Performance ✓ — 30min timeout is generous; trap + belt-and-suspenders down.sh prevents leaked compose networks across runner reuse. Paths filter excludes doc-only changes, so cost stays tied to what actually needs the harness.
Observation (non-blocking) — The "Dump compose logs on failure" step swallows individual docker compose logs <svc> failures with || true. If the compose network itself is down (the failure mode that motivates the dump), all four service logs will fail in series and you'll only see four "Error: No such service" lines. Not blocking — just consider a single docker compose -f compose.yml logs --no-color as the lead, with per-service tails as fallback. Defer to author judgment.
LGTM in concept. Workflow-touch → no auto-approval per spec.
First run on PR #2410 failed with 'container harness-tenant-1 is unhealthy' but the dump-compose-logs step printed empty tenant logs because run-all-replays.sh's trap-on-EXIT had already torn down the harness. Setting KEEP_UP=1 leaves containers in place; the always-run Force teardown step at the end owns cleanup explicitly. Now we'll actually see why the tenant didn't become healthy. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…tenant boots Found via the first run of the harness-replays-required-check workflow (#2410): the tenant container failed its healthcheck after 100s with "refusing to boot without encryption in production". This is the deferred CRITICAL flagged on PR #2401 — `crypto.InitStrict()` requires SECRETS_ENCRYPTION_KEY when MOLECULE_ENV=production, and the harness sets prod-mode but never seeded a key. Fix: add a clearly-test 32-byte base64 value (encoding the literal string "harness-test-only-not-for-prod!!") inline. Keeping MOLECULE_ENV=production preserves the harness's value as a production- shape replay surface — it now exercises the full encryption boot path including the strict check, rather than skirting it via dev-mode. Why inline rather than .env: - The harness compose file is meant to be self-contained and reproducible from a clean clone. An external .env would split the config across two files for one synthetic value. - The value is intentionally a sentinel; there's no operator decision here to gate behind a per-deployment file. After this lands the harness boots clean and `run-all-replays.sh` can exercise the buildinfo + peer-discovery replays as designed. The required-check workflow itself (#2410) needs no change.
|
Self-review caught the harness-tenant unhealthy failure on the workflow's first run — same root cause as the deferred CRITICAL from PR #2401 review: The harness sets CI re-running on the new SHA. Will approve+merge once green. Cross-reference: this closes the gap I flagged on #2401 — the harness was a missing instance of that check (production binary refuses to boot without the key, but no test invoked production-mode boot until this PR added one). |
…coding Replaces the hardcoded base64 sentinel (630dd0d) with a per-run generation in up.sh, exported into compose's interpolation environment. Why: - Hardcoding a 32-byte base64 string in the repo, even one labelled "test-only", sets a bad muscle-memory pattern. The next agent or contributor copies the shape into another harness — or worse, into a staging .env — and the test-only sentinel turns into something someone treats as a real key. - Secret scanners flag key-shaped values regardless of the surrounding comment claiming intent. Avoiding the literal entirely sidesteps the false-positive. - A fresh key per harness lifetime more closely mimics prod's per-tenant isolation, exercising the same code paths without any pretense of stable encrypted-data fixtures (which the harness wipes on every ./down.sh anyway). Implementation: - up.sh: `openssl rand -base64 32` if SECRETS_ENCRYPTION_KEY isn't already set in the caller's env. Honoring a pre-set value lets a debug session pin a key for reproducibility (e.g. when investigating encrypted-row corruption). - compose.yml: `${SECRETS_ENCRYPTION_KEY:?…}` makes a misuse loud — running `docker compose up` directly bypassing up.sh fails fast with a clear error pointing at the right entry point, rather than a 100s unhealthy-tenant timeout. Both paths verified via `docker compose config`: - with key exported: value interpolates cleanly - without it: "required variable SECRETS_ENCRYPTION_KEY is missing a value: must be set — run via tests/harness/up.sh, which generates one per run"
peer-discovery-404 imports workspace/a2a_client.py which depends on httpx; the runner's stock Python doesn't have it, so the replay's PARSE assertion (b) fails with ModuleNotFoundError on every run. The WIRE assertion (a) — pure curl — passes, so the failure was masking just enough to make the replay LOOK partially-broken when the tenant side is fine. Adding tests/harness/requirements.txt with only httpx instead of sourcing workspace/requirements.txt: that file pulls a2a-sdk, langchain-core, opentelemetry, sqlalchemy, temporalio, etc. — ~30s of install for one replay's PARSE step. The harness's deps surface should grow when a new replay introduces a new import, not by default. Workflow gains one step (`pip install -r tests/harness/requirements.txt`) between the /etc/hosts setup and run-all-replays. No other changes.
…ta_persistence) in GET' (#2410) from fix/compute-serialize-provider into main
Summary
Phase 2 of the harness roadmap (per
tests/harness/README.md): make harness-based replays a required CI check on every PR touching the tenant binary or the harness itself.Triggers
tests/harness/run-all-replays.shon push + pull_request to staging+main, paths-filtered toworkspace-server/**,canvas/**,tests/harness/**, and this workflow.Why now
2026-04-30 we shipped #2398 which added
/buildinfoas a public route inrouter.gobut never added it to TenantGuard's allowlist. The handler-level test inbuildinfo_test.goconstructs a minimal gin engine without TenantGuard — green. The harness'sbuildinfo-stale-image.shreplay would have caught it (cf-proxy doesn't injectX-Molecule-Org-Id, so the curl path is identical to production's redeploy verifier). Nobody ran the harness pre-merge.Result: bug shipped, the redeploy verifier silently soft-warned every tenant as "unreachable" for ~1 day before it was traced (PR #2409 fixes the allowlist).
This gate makes "did you actually run the harness?" a CI invariant instead of memory discipline.
Pipeline
Single-job-with-conditional-steps pattern (matches
e2e-api.yml). One check run regardless of paths-filter outcome — branch-protection-clean per the PR #2264 SKIPPED-in-set finding.merge_grouptrigger included so this is branch-protectable when ready.What runs today
replays/buildinfo-stale-image.sh— would have caught feat(deploy): verify each tenant /buildinfo matches published SHA after redeploy #2398's TenantGuard gapreplays/peer-discovery-404.sh— closes list_peers + get_peers swallow non-200 + exceptions, leaving agent + user blind to root cause #2397Both run automatically. Adding a new replay needs no per-replay registration —
run-all-replays.shpicks up everyreplays/*.sh.Build cost
The tenant image is the full
Dockerfile.tenant(Go + Next.js + template clones from GitHub). Each PR currently rebuilds from scratch —timeout-minutes: 30ceiling. Acceptable as a Phase 2 floor; can wire buildx GHA cache later if it becomes painful.Follow-ups (separate PRs)
Harness Replaysto staging branch protection once the workflow has been green for several PRs (the new-workflow-instability hedge other gates followed).Test plan
harness-replaysend-to-end and goes greenworkspace-server/triggers this workflow🤖 Generated with Claude Code