Add real-model browser E2E nightly tests with reusable scenarios #204

Activity

1. hsliuustc0106 commented on Oct 17, 2026

   

   hsliuustc0106

   on Oct 17, 2026

   ContributorAuthor

   More actions

   Review: readiness for an implementer

   Automated review, not a human sign-off. Basis: this issue text as of 2026-10-18, main at 4470c1e (the pilot's 0052df0 is an ancestor). I checked the claims against the repo instead of trusting them.

   Verdict: ready to schedule after four decisions. The scenario set and the result assertions are sound. Everything still undefined is about how the suite runs in CI, and each item changes the implementation shape — so decide them here, not in review.

   What checks out

   • Fixture math is right: paid = A001/A002/A004/A005 → 4 / 400; paid ∧ 华东 = A001/A004 → 2 / 140.
   • The pilot's tip 0052df0 is on main, and the runtime matches the pin in core/dev/harness/package.json (0.2.0-rc.2).
   • The pilot's route is real in that pinned DSH: provider deepseek-official plus model deepseek-flash exist in llm-deepseek-api-key / llm-deepseek, and DSH reads DEEPSEEK_API_KEY (and DEEPSEEK_BASE_URL) from the environment — so no DSH patch is needed, which satisfies the hard rule.
   • Separating this from nightly.yml (compat matrix) and from PR checks is the right call.

   Decide before implementation

   1. The launcher is coupled to a DSH checkout; CI installs DSH from npm.
   bench.sh starts pnpm -s dsh … --patch apps/web/tests/pin-browse-picker.overlay.yml from a deepseek-harness checkout. That overlay is a checkout-only test fixture: it pins the in-browser directory picker, because the native OS chooser cannot be driven by a browser automation page. Scenario 1 (attachment upload) depends on it. CI installs @deepseek-ai/dsh into core/dev/harness, where neither that path nor pnpm exists. Choose one and say so in this issue: (a) the workflow clones the harness at the pinned tag, or (b) the suite runs the npm-installed dsh with a FlyBot-owned overlay that disables and re-inserts the browse-picker rows.

   2. No browser driver is declared in this repo.
   lib.mjs resolves playwright-core out of a DSH checkout's pnpm store and says outright that "ds-bot installs no browser tooling of its own". CI has Chrome on ubuntu-latest, but no such store. Adding a pinned playwright-core (or driving Chrome over CDP) is a decision against that stated invariant, and it belongs here rather than in a review comment later.

   3. Secret provisioning and hygiene.
   At review time this repository has zero Actions secrets configured (GET /repos/.../actions/secrets → total_count: 0), so DEEPSEEK_API_KEY has to be added before the first scheduled run — say who does that. The repo is public, so Actions logs and artifacts are world-readable and a leak is permanent. The issue should require, concretely: the secret-bearing job runs on main only (if: github.ref == 'refs/heads/main', never pull_request), permissions: contents: read, web.log and diagnostics sanitized (?token= is written there — see bench.sh and the login regex in smoke.mjs), an explicit artifact allow-list, and no raw DSH_HOME upload. Also add a hard guard that refuses to start when DSH_HOME is unset or resolves to ~/.dsh; today that is prose only.

   4. Where the code lives — and therefore which gates the PR hits.
   core/dev/ is owned by ds-bot-core.md, so a live runner there forces a doc update and makes every edit flow through patch generation. Root dev/ is now an established home for dev-only harnesses (see below). State the choice; the two options have different review surfaces.

   Reuse the interop harness pattern (#201)

   main just gained dev/interop/ (#201): a development-only, runtime-agnostic harness with a no-network tripwire, fake clock, fresh tmpdir per run, and keyless tests wired into root npm run verify. This issue describes the same shape — shared scenarios and assertions, application-specific adapter, separate live runner — and should follow it rather than invent a second convention:

   • put fixture/scenario/assertion modules and their keyless unit tests (a fake adapter) in the same style, so ordinary PR CI covers the logic;
   • keep the live command out of npm test / npm run verify (the core glob is core/test/*.test.js);
   • put the adapter contract next to the existing interop contract (docs/interop/ already holds the OpenCode pilot contract), and say how the two relate — otherwise two "adapter contract" documents will coexist.

   Tighten these ambiguities

   • "Keep the original unchanged" names no file. Say exactly: the uploaded CSV in the run workspace, asserted by SHA-256 before/after, and summary.json likewise unmodified (scenario 4 compares it).
   • JSON comparisons: compare parsed JSON, not bytes — indentation, key order and \u escaping will differ. Keep byte-for-byte only for the input CSV. Assert count and total are numbers.
   • "Fresh run IDs/data" conflicts with a fixed fixture. Keep the CSV fixed (400/140 depend on it); freshness means new run ID, DSH_HOME and workspace per run.
   • Prove the run is real: assert the resolved route/base URL is not loopback and the key is not mock-key, and record the model id. That is what actually enforces "mock suites remain separate".
   • Budgets need default numbers to be reviewable: e.g. per-check ≤ 180 s, job timeout ≤ 20 min, per-run token cap ~250K (the pilot used ~116K), and zero retries inside a run.
   • Outcome/exit contract: pass | assertion_failed | provider_failed | infra_failed | budget_exceeded, mapped to exit codes, plus the report schema (run ID, ISO-8601 timestamps with offset, monotonic per-check durations, recorded config, usage).
   • Workflow conventions: separate file, actionlint-clean (the package job lints workflows in test.yml), concurrency with cancel-in-progress: false, a UTC cron slot distinct from compat's 16:00Z, commented in Asia/Shanghai, not added to tests-ok or branch protection, and optionally the same "window check" gate so no tokens are spent when main has no new commits.
   • Orbit is unverifiable right now: nothing in this repo references Orbit, so "remaining work to run the same cases on Orbit" cannot be checked. Link its entrypoint, or scope this to the contract + FlyBot adapter and file Orbit integration as a follow-up — same pattern as #198, marked not exercised.
   • Attach the pilot evidence: the runner is not committed and the report lacks absolute timestamps. Without the sanitized structured report/transcript attached here, the implementer re-derives the format this issue asks them to match.

   Suggested acceptance criteria additions

   • The suite runs in CI without a deepseek-harness checkout, or the workflow provisions one at the pinned tag — state which, and how the directory-picker pin is applied.
   • The browser driver is a pinned, repo-declared dependency, with its Chrome source documented.
   • The run fails closed when the resolved provider is not the real endpoint, or when DEEPSEEK_API_KEY is missing or a placeholder — never reported as a pass.
   • DEEPSEEK_API_KEY is provisioned as a repository secret; the secret-bearing job runs on main only, and artifacts pass a sanitizer that rejects token=, key material and cookies.
   • Shared scenarios/assertions are covered by keyless tests with a fake adapter, following the dev/interop pattern, so npm run verify stays green and meaningful.
   • "Stable" means N consecutive scheduled runs at one recorded configuration (e.g. 5); retries are never silent.

   No repository files were changed by this review.
2. hsliuustc0106 commented on Oct 17, 2026

   

   hsliuustc0106

   on Oct 17, 2026

   ContributorAuthor

   More actions

   Decisions on the four blockers, per the readiness review (to be delivered by the implementation PR):

   1. Launcher/CI: the workflow provisions a deepseek-harness checkout at the pinned tag (derived from core/dev/harness/package.json → dsh-v<version>), exactly like the local bench, and the upstream apps/web/tests/pin-browse-picker.overlay.yml applies the directory-picker pin. No FlyBot-owned overlay; the suite never runs from the npm-installed CLI.
   2. Browser driver: playwright-core resolves from that same harness checkout through the existing dev/desktop-linux/lib.mjs helper — the repo still declares no browser tooling of its own. Chrome: CHROME_BIN, else google-chrome/chromium(-browser) on PATH (ubuntu-latest ships Chrome), else playwright's download; documented in the suite README.
   3. Secrets: DEEPSEEK_API_KEY is provisioned by the maintainer as a repository secret before the first scheduled run (this repo currently has zero secrets). The secret-bearing job runs on main only (schedule + dispatch guarded by github.ref == 'refs/heads/main'; never pull_request), permissions: contents: read, evidence passes a sanitizer that fails on token=, the secret's value or cookie material, and only an explicit allow-list is uploaded (never a raw DSH_HOME). The runner fails closed when the key is missing/mock-key/placeholder, when the resolved base URL is loopback, or when DSH_HOME is unset or resolves under ~/.dsh.
   4. Location: dev/live-e2e/, beside dev/interop/ (Add the offline cross-runtime collaboration harness #201): fixture/scenario/assertion modules with keyless fake-adapter tests wired into root npm run verify; the live runner (bench launcher, adapter, reporter) runs only via the documented command and the nightly/dispatch workflow. The adapter contract goes to docs/interop/ next to the OpenCode pilot contract, with a section relating the two. Orbit integration is filed as a follow-up marked not exercised.

   Tightenings adopted: CSV immutability asserted by SHA-256 (the uploaded copy in the run workspace, and summary.json across later scenarios); JSON compared parsed with count/total required to be numbers, byte-for-byte only for the CSV; freshness = new run id + fresh DSH_HOME/workspace with the fixture CSV fixed; the resolved route must be non-loopback and the key non-mock, recorded in the report; budgets default to per-check ≤ 180 s, job ≤ 20 min, ≤ 250K tokens/run, zero retries; outcome contract pass | assertion_failed | provider_failed | infra_failed | budget_exceeded mapped to exit codes 0–4 with a structured report (run id, ISO-8601 timestamps with offset, monotonic per-check durations, config, usage); workflow: own file, actionlint-clean, concurrency with cancel-in-progress: false, cron 30 16 * * * UTC (00:30 Asia/Shanghai, clear of compat's 16:00Z), window gate, never added to tests-ok.

   Maintainer asks (not blockers for the PR): attach the sanitized pilot report/transcript here, and add the DEEPSEEK_API_KEY secret before the first scheduled run.
3. hsliuustc0106

   added 2 commits that reference this issue on Oct 17, 2026

   Add the real-model browser E2E suite of #204

   ...

   c02417d

   Add the real-model browser E2E suite of #204

   ...

   aaf69f1
4. hsliuustc0106

   closed this as completedin #225on Oct 18, 2026