From c3998dde7c6bcdac5dd72e33e9fa667519b26697 Mon Sep 17 00:00:00 2001 From: imccyu Date: Mon, 26 Oct 2026 22:54:02 +0800 Subject: [PATCH 1/6] feat(ci): run one script for the merged tree in the pre-merge check `Policy / Pre-Merge Check` now runs `scripts/pre-merge-check.ts` under plain Node and knows nothing else about the merged tree. That script installs the dependencies, checks that every released manifest carries the root version while the install runs, and builds the Host and Client contracts, reporting which step failed. --- .github/AGENTS.md | 2 + .github/workflows/pr-check-on-head-branch.yml | 21 ++- scripts/ci-workflow.spec.ts | 4 +- scripts/pre-merge-check.spec.ts | 70 ++++++++++ scripts/pre-merge-check.ts | 117 ++++++++++++++++ scripts/verify-version-sync.spec.ts | 74 ++++++++++ scripts/verify-version-sync.ts | 128 ++++++++++++++++++ 7 files changed, 407 insertions(+), 9 deletions(-) create mode 100644 scripts/pre-merge-check.spec.ts create mode 100644 scripts/pre-merge-check.ts create mode 100644 scripts/verify-version-sync.spec.ts create mode 100644 scripts/verify-version-sync.ts diff --git a/.github/AGENTS.md b/.github/AGENTS.md index 9b891d3a6e..e0ff76ca1f 100644 --- a/.github/AGENTS.md +++ b/.github/AGENTS.md @@ -2,6 +2,8 @@ Run jobs on Windows runners (`windows-*` labels) under native `pwsh`. Native Windows pull-request jobs live in `ci-windows.yml` and Python pull-request jobs in `ci-python.yml`, so each PR workflow can be re-run independently; their checks feed the required `windows checks passed` and `python checks passed` verdicts, as Linux jobs in `ci.yml` feed `all checks passed`; Wine runs Windows Node on hosted Linux only in `ci-master.yml`. Python runtime CI checks Linux/Windows x64 on pull requests and Linux ARM64 plus both macOS architectures on master pushes; releases retain all five targets ([platform policy](workflows/ci-master.yml)). `ci.yml`, `ci-windows.yml`, and `ci-python.yml` are pull-request-only. Master-only platform checks, Linux/Windows self-hosted standbys, and manual runner benchmarks live in `ci-master.yml`, which listens to master pushes and `workflow_dispatch`, not `pull_request`; separating workflow triggers keeps master-only jobs out of PR check panels. The master standbys validate the self-hosted failover targets; preserve the existing per-platform switches (values `selfhosted` for the in-house standbys and `blacksmith` for Blacksmith's hosted runners) and the Dependabot hosted fallback under the `selfhosted` values ([failover runbook](../.agents/notes/implemented/process/2026-07-26-ci-failover-runbook.md), [blacksmith failover leg note](../.agents/notes/implemented/process/2026-09-09-blacksmith-failover-leg.md)). +`Policy / Pre-Merge Check` runs `scripts/pre-merge-check.ts` under plain Node and knows nothing else about the merged tree: that script installs, checks the released manifests carry the root version, and builds the Host and Client contracts, so change it in one pull request together with the build it orchestrates. + The manual [Windows packaging workflow](workflows/windows-package.yml) uses the dedicated self-hosted Windows x64 label `dsh-win-package-trial` under native `pwsh`; preserve this routing separately from the automatic CI lanes. The merge checks run from three workflows named `Policy`, which run on `pull_request_target` so their definitions come from master, and call the `deepseek-harness-check` action directly: `pr-check-on-head-branch.yml` on pull-request updates gives `Policy / Master Change` and `Policy / Pre-Merge Check`, `pr-check-after-approve.yml` on pull-request updates and submitted or dismissed reviews gives `Policy / Approve Check` and `Policy / Reviewer Evaluate`, and `pr-check-on-base-branch.yml` on master pushes re-runs the head-branch checks as `Policy / Recheck`. Branch rules require those check names, so keep the job names and the workflow name. Each check reads its mode from a repository variable: `MERGE_POLICY_MASTER_DIFF`, `MERGE_POLICY_PRE_MERGE`, `MERGE_POLICY_HEAD_DIFF`, and `MERGE_POLICY_REVIEW`, each `off` (skip), `report` (run without failing the job), or `block` (fail the job); a missing value or an unknown one counts as `report`. `Policy / Pre-Merge Check` merges the commit `Policy / Master Change` reports, so it runs only after that job and stays off while `MERGE_POLICY_MASTER_DIFF` is `off`; a merge it cannot do and a build that fails both follow its own mode, so `report` warns and skips the build while `block` fails the job. diff --git a/.github/workflows/pr-check-on-head-branch.yml b/.github/workflows/pr-check-on-head-branch.yml index 5ab02a6a94..87298d5534 100644 --- a/.github/workflows/pr-check-on-head-branch.yml +++ b/.github/workflows/pr-check-on-head-branch.yml @@ -93,15 +93,22 @@ jobs: node-version: '24' package-manager-cache: false - - name: Run the pre-merge command + - name: Run the tree's pre-merge check if: steps.merge.outputs.merged == 'true' env: CHECK_MODE: ${{ vars.MERGE_POLICY_PRE_MERGE }} run: | - if ! bash -euxo pipefail -c 'pnpm install --frozen-lockfile && pnpm exec tsx scripts/compile-referenced-projects.ts host-contracts && pnpm exec tsx scripts/compile-referenced-projects.ts client'; then - if [ "$CHECK_MODE" = block ]; then - echo "::error title=Pre-Merge Check::the merged tree does not install or compile" - exit 1 - fi - echo "::warning title=Pre-Merge Check::the merged tree does not install or compile; this check does not block" + # The check names one file and knows nothing else about how the merged tree builds itself. + if [ ! -f scripts/pre-merge-check.ts ]; then + echo "This tree has no scripts/pre-merge-check.ts; the check is skipped." + exit 0 + fi + if node scripts/pre-merge-check.ts; then + exit 0 + fi + if [ "$CHECK_MODE" = block ]; then + echo "::error title=Pre-Merge Check::the merged tree fails its pre-merge check" + exit 1 + fi + echo "::warning title=Pre-Merge Check::the merged tree fails its pre-merge check; this check does not block" fi diff --git a/scripts/ci-workflow.spec.ts b/scripts/ci-workflow.spec.ts index 877b45c610..3fe2c7c340 100644 --- a/scripts/ci-workflow.spec.ts +++ b/scripts/ci-workflow.spec.ts @@ -1716,7 +1716,7 @@ describe('Policy workflows', () => { it('keeps the pre-merge build behind the merge step, which honours the mode on its failures', () => { const steps = stepsOf(workflowJob(loadWorkflow('.github/workflows/pr-check-on-head-branch.yml'), 'pre-merge')) const merge = steps.findIndex(step => step.id === 'merge') - const build = steps.findIndex(step => step.name === 'Run the pre-merge command') + const build = steps.findIndex(step => step.name === "Run the tree's pre-merge check") expect(merge).toBeGreaterThanOrEqual(0) expect(build).toBeGreaterThan(merge) const merging = steps[merge] @@ -1727,7 +1727,7 @@ describe('Policy workflows', () => { expect(step.run).toContain('"$CHECK_MODE" = block') } for (const step of steps.slice(merge + 1)) { - if (step.name === 'Run the pre-merge command') continue + if (step.name === "Run the tree's pre-merge check") continue expect(step.if).toBe("steps.merge.outputs.merged == 'true'") } }) diff --git a/scripts/pre-merge-check.spec.ts b/scripts/pre-merge-check.spec.ts new file mode 100644 index 0000000000..a47fe96d9c --- /dev/null +++ b/scripts/pre-merge-check.spec.ts @@ -0,0 +1,70 @@ +import { mkdirSync, mkdtempSync, rmSync, writeFileSync } from 'node:fs' +import { tmpdir } from 'node:os' +import { dirname, join } from 'node:path' +import { describe, expect, it } from 'vitest' +import { buildSteps, preMergeCheck, type PreMergePlan } from './pre-merge-check.ts' + +/** + * Build a repository holding the given manifests. + * @param files - Manifest path to version. + * @returns The temporary root. + */ +function repository(files: Record): string { + const root = mkdtempSync(join(tmpdir(), 'dsh-pre-merge-')) + for (const [path, version] of Object.entries(files)) { + const absolute = join(root, path) + mkdirSync(dirname(absolute), { recursive: true }) + writeFileSync(absolute, JSON.stringify({ name: 'x', version })) + } + return root +} + +/** + * A plan whose commands exit with the given codes, so the check runs without pnpm. + * @param codes - Exit code of the install and of each build. + * @returns The plan. + */ +function plan(codes: readonly number[]): PreMergePlan { + const step = (name: string, code: number) => ({ name, command: process.execPath, args: ['-e', `process.exit(${code})`] }) + const [install, ...builds] = codes + return { install: step('install', install ?? 0), steps: builds.map((code, index) => step(`build-${index}`, code)) } +} + +describe('pre-merge check', () => { + it('passes an install, a version check, and the builds that all succeed', async () => { + const root = repository({ 'package.json': '1.0.0', 'packages/core/session/package.json': '1.0.0' }) + try { + const lines: string[] = [] + expect(await preMergeCheck(root, { plan: plan([0, 0, 0]), report: line => void lines.push(line) })).toEqual({ ok: true }) + expect(lines[0]).toBe('verify-version-sync: 1 released manifest(s) carry 1.0.0') + } finally { + rmSync(root, { recursive: true, force: true }) + } + }) + + it('reports a failed install without running the builds', async () => { + const root = repository({ 'package.json': '1.0.0', 'packages/core/session/package.json': '1.0.0' }) + try { + const lines: string[] = [] + const outcome = await preMergeCheck(root, { plan: plan([2, 0]), report: line => void lines.push(line) }) + expect(outcome).toEqual({ ok: false, reason: 'the install failed' }) + expect(lines.some(line => line.startsWith('pre-merge-check: building'))).toBe(false) + } finally { + rmSync(root, { recursive: true, force: true }) + } + }) + + it('fails a tree whose released manifests carry another version', async () => { + const root = repository({ 'package.json': '2.0.0', 'packages/core/session/package.json': '1.0.0' }) + try { + const outcome = await preMergeCheck(root, { plan: plan([0, 0, 0]), report: () => {} }) + expect(outcome).toEqual({ ok: false, reason: 'the released manifests do not carry the root version' }) + } finally { + rmSync(root, { recursive: true, force: true }) + } + }) + + it('builds the host contracts before the client', () => { + expect(buildSteps().map(step => step.name)).toEqual(['host contracts', 'client']) + }) +}) diff --git a/scripts/pre-merge-check.ts b/scripts/pre-merge-check.ts new file mode 100644 index 0000000000..687e9162d7 --- /dev/null +++ b/scripts/pre-merge-check.ts @@ -0,0 +1,117 @@ +/** + * Validate the tree a pull request produces once merged with its base branch: the install, + * the version synchronization of the released manifests, and the Host and Client builds. + * + * The pull-request check runs this through `pnpm run pre-merge-check` and knows only that + * script's name, so what the merged tree needs changes here without touching the check. The + * script itself runs under plain Node: it spawns pnpm, so no dependency is installed first. + */ + +import { spawn, spawnSync } from 'node:child_process' +import { resolve } from 'node:path' +import process from 'node:process' +import { pathToFileURL } from 'node:url' +import { describeVersions, readVersions } from './verify-version-sync.ts' + +/** One command of the check. */ +export interface PreMergeStep { + /** Short name for the log. */ + readonly name: string + readonly command: string + readonly args: readonly string[] +} + +/** What the check runs: the install every other step depends on, then the builds in order. */ +export interface PreMergePlan { + readonly install: PreMergeStep + readonly steps: readonly PreMergeStep[] +} + +/** + * The commands that build the merged tree, in order. + * @returns Steps after the install. + */ +export function buildSteps(): PreMergeStep[] { + return [ + { name: 'host contracts', command: 'pnpm', args: ['exec', 'tsx', 'scripts/compile-referenced-projects.ts', 'host-contracts'] }, + { name: 'client', command: 'pnpm', args: ['exec', 'tsx', 'scripts/compile-referenced-projects.ts', 'client'] }, + ] +} + +/** The install every other step depends on. */ +export const INSTALL_STEP: PreMergeStep = { name: 'install', command: 'pnpm', args: ['install', '--frozen-lockfile'] } + +/** The plan the repository uses. */ +export const DEFAULT_PLAN: PreMergePlan = { install: INSTALL_STEP, steps: buildSteps() } + +/** Options of the check. */ +export interface PreMergeOptions { + /** Sink for the check's own lines. */ + readonly report?: (line: string) => void + /** Plan to run; the repository's own plan when absent. */ + readonly plan?: PreMergePlan +} + +/** What the check decided. */ +export interface PreMergeOutcome { + readonly ok: boolean + /** Why the tree failed, when it did. */ + readonly reason?: string +} + +/** + * Run one step to completion with the caller's terminal, so its output lands in the job log. + * @param step - Step to run. + * @param root - Working directory. + * @returns Its exit code, or 1 when a signal ended it. + */ +function run(step: PreMergeStep, root: string): number { + const result = spawnSync(step.command, [...step.args], { cwd: root, stdio: 'inherit' }) + return result.status ?? 1 +} + +/** + * Check the merged tree, reporting why it failed. + * @param root - Repository root. + * @param options - Sink for the check's own lines, and the plan to run. + * @returns Whether the tree passed, with the reason when it did not. + */ +export async function preMergeCheck(root: string, options: PreMergeOptions = {}): Promise { + const report = options.report ?? ((line: string): void => { + process.stdout.write(`${line}\n`) + }) + const plan = options.plan ?? DEFAULT_PLAN + // The manifests are read while the install runs: the version check needs no installed dependency. + const install = spawn(plan.install.command, [...plan.install.args], { cwd: root, stdio: 'inherit' }) + const versions = readVersions(root) + const lines = describeVersions(versions) + const installCode = await new Promise((settle) => { + install.once('close', (code) => { + settle(code ?? 1) + }) + install.once('error', () => { + settle(1) + }) + }) + const versionsOk = versions.rootVersion !== undefined && lines.length === 1 + for (const line of lines) report(`verify-version-sync: ${line}`) + if (installCode !== 0) return { ok: false, reason: 'the install failed' } + for (const step of plan.steps) { + report(`pre-merge-check: building ${step.name}`) + if (run(step, root) !== 0) return { ok: false, reason: `the ${step.name} build failed` } + } + if (!versionsOk) return { ok: false, reason: 'the released manifests do not carry the root version' } + return { ok: true } +} + +const invokedPath = process.argv[1] +const isMain = invokedPath !== undefined && import.meta.url === pathToFileURL(resolve(invokedPath)).href +if (isMain) { + const outcome = await preMergeCheck(resolve(import.meta.dirname, '..')) + if (outcome.ok) { + process.stdout.write('pre-merge-check: passed\n') + } else { + process.stderr.write(`pre-merge-check: failed: ${outcome.reason ?? 'unknown reason'}\n`) + process.exitCode = 1 + } +} diff --git a/scripts/verify-version-sync.spec.ts b/scripts/verify-version-sync.spec.ts new file mode 100644 index 0000000000..e928e2ab7a --- /dev/null +++ b/scripts/verify-version-sync.spec.ts @@ -0,0 +1,74 @@ +import { mkdirSync, mkdtempSync, rmSync, writeFileSync } from 'node:fs' +import { tmpdir } from 'node:os' +import { dirname, join } from 'node:path' +import { describe, expect, it } from 'vitest' +import { describeVersions, mismatchedVersions, readVersions, releasedManifests } from './verify-version-sync.ts' + +/** + * Build a repository holding the given manifests. + * @param files - Manifest path to version, or to `null` for a manifest without one. + * @returns The temporary root. + */ +function repository(files: Record): string { + const root = mkdtempSync(join(tmpdir(), 'dsh-version-sync-')) + for (const [path, version] of Object.entries(files)) { + const absolute = join(root, path) + mkdirSync(dirname(absolute), { recursive: true }) + writeFileSync(absolute, JSON.stringify(version === null ? { name: 'x' } : { name: 'x', version })) + } + return root +} + +describe('released manifests', () => { + it('collects the packages and applications of the root version and skips other version lines', () => { + const root = repository({ + 'package.json': '1.0.0', + 'packages/core/session/package.json': '1.0.0', + 'packages/AGENTS.md': null, + 'apps/cli/package.json': '1.0.0', + 'vendor/cordis/package.json': '4.0.5', + 'native/system/package.json': '0.1.2', + }) + try { + expect(releasedManifests(root)).toEqual(['apps/cli/package.json', 'packages/core/session/package.json']) + } finally { + rmSync(root, { recursive: true, force: true }) + } + }) +}) + +describe('version synchronization', () => { + it('names a manifest left on another version or without one', () => { + const root = repository({ + 'package.json': '2.0.0', + 'packages/core/session/package.json': '2.0.0', + 'packages/core/agent/package.json': '1.0.0', + 'apps/cli/package.json': null, + }) + try { + expect(describeVersions(readVersions(root))).toEqual([ + '2 of 3 released manifest(s) do not carry the root version 2.0.0:', + ' apps/cli/package.json: no version', + ' packages/core/agent/package.json: 1.0.0', + ]) + } finally { + rmSync(root, { recursive: true, force: true }) + } + }) + + it('accepts a tree whose released manifests all carry the root version', () => { + const root = repository({ + 'package.json': '3.0.0', + 'packages/core/session/package.json': '3.0.0', + 'apps/web/package.json': '3.0.0', + 'vendor/cordis/package.json': '4.0.5', + }) + try { + const report = readVersions(root) + expect(mismatchedVersions(report.rootVersion ?? '', report.manifests)).toEqual([]) + expect(describeVersions(report)).toEqual(['2 released manifest(s) carry 3.0.0']) + } finally { + rmSync(root, { recursive: true, force: true }) + } + }) +}) diff --git a/scripts/verify-version-sync.ts b/scripts/verify-version-sync.ts new file mode 100644 index 0000000000..b94ae39dac --- /dev/null +++ b/scripts/verify-version-sync.ts @@ -0,0 +1,128 @@ +/** + * Check that every released manifest carries the root version. A release + * synchronization bumps them together, so a mismatch means a merge combined two + * version lines: a pull request that bumps versions merged onto a master that + * added a package, for instance. + * + * The check reads manifests only, so it needs no installed dependency. + */ + +import { readFileSync, readdirSync, statSync } from 'node:fs' +import { join, resolve } from 'node:path' +import process from 'node:process' +import { pathToFileURL } from 'node:url' + +/** One manifest's version, with `undefined` when the file does not declare one. */ +export interface ManifestVersion { + /** Repository-relative path of the manifest. */ + readonly path: string + readonly version: string | undefined +} + +/** The root version and every released manifest's version. */ +export interface VersionReport { + readonly rootVersion: string | undefined + readonly manifests: ManifestVersion[] +} + +/** Directories that hold released manifests below the repository root. */ +const MANIFEST_DIRECTORIES = ['packages', 'apps'] as const + +/** Directories whose manifests follow another version line. */ +const EXCLUDED_DIRECTORIES = ['vendor', 'native'] as const + +/** + * Read the version one `package.json` declares. + * @param path - Absolute path of the manifest. + * @returns The declared version, or `undefined` when it declares none. + */ +function versionOf(path: string): string | undefined { + const manifest: unknown = JSON.parse(readFileSync(path, 'utf8')) + if (typeof manifest !== 'object' || manifest === null) return undefined + const version: unknown = (manifest as { version?: unknown }).version + return typeof version === 'string' ? version : undefined +} + +/** + * Collect the manifests that must carry the root version: two levels under each + * workspace directory, so `packages//` and `apps/`. + * @param root - Repository root. + * @returns Manifest paths relative to the root, sorted. + */ +export function releasedManifests(root: string): string[] { + const paths: string[] = [] + for (const directory of MANIFEST_DIRECTORIES) { + let groups: string[] + try { + groups = readdirSync(join(root, directory)) + } catch { + continue + } + for (const group of groups) { + if ((EXCLUDED_DIRECTORIES as readonly string[]).includes(group)) continue + const groupDirectory = join(root, directory, group) + if (!statSync(groupDirectory).isDirectory()) continue + const manifest = join(groupDirectory, 'package.json') + try { + if (statSync(manifest).isFile()) paths.push(`${directory}/${group}/package.json`) + } catch { + // A directory without a manifest is not a package. + } + for (const entry of readdirSync(groupDirectory)) { + const nested = `${directory}/${group}/${entry}/package.json` + try { + if (statSync(join(root, nested)).isFile()) paths.push(nested) + } catch { + // Not a package directory. + } + } + } + } + return paths.sort() +} + +/** + * Compare every released manifest with the root version. + * @param rootVersion - Version of the root `package.json`. + * @param manifests - Each manifest's path and version. + * @returns The manifests that carry another version or none, in path order. + */ +export function mismatchedVersions(rootVersion: string, manifests: readonly ManifestVersion[]): ManifestVersion[] { + return manifests.filter(manifest => manifest.version !== rootVersion).sort((left, right) => left.path.localeCompare(right.path)) +} + +/** + * Read the root version and the released manifests of a repository. + * @param root - Repository root. + * @returns Root version and manifests. + */ +export function readVersions(root: string): VersionReport { + const rootVersion = versionOf(join(root, 'package.json')) + const manifests = releasedManifests(root).map(path => ({ path, version: versionOf(join(root, path)) })) + return { rootVersion, manifests } +} + +/** + * Describe a version mismatch for a reader. + * @param report - Root version and manifests. + * @returns One line when the tree is consistent, otherwise a header and one line per mismatched manifest. + */ +export function describeVersions(report: VersionReport): string[] { + const { rootVersion, manifests } = report + if (rootVersion === undefined) return ['the root package.json declares no version'] + const mismatched = mismatchedVersions(rootVersion, manifests) + if (mismatched.length === 0) return [`${manifests.length} released manifest(s) carry ${rootVersion}`] + return [ + `${mismatched.length} of ${manifests.length} released manifest(s) do not carry the root version ${rootVersion}:`, + ...mismatched.map(manifest => ` ${manifest.path}: ${manifest.version ?? 'no version'}`), + ] +} + +const invokedPath = process.argv[1] +const isMain = invokedPath !== undefined && import.meta.url === pathToFileURL(resolve(invokedPath)).href +if (isMain) { + const lines = describeVersions(readVersions(resolve(import.meta.dirname, '..'))) + const failed = lines.length > 1 || lines[0]?.includes('declares no version') === true + for (const line of lines) (failed ? console.error : console.log)(`verify-version-sync: ${line}`) + if (failed) process.exitCode = 1 +} From a518d10c9559417d23536dc483cce8e64acacbe7 Mon Sep 17 00:00:00 2001 From: imccyu Date: Mon, 26 Oct 2026 23:12:29 +0800 Subject: [PATCH 2/6] fix(ci): keep the pre-merge check's failures honest The missing-script branch now fails in block mode instead of passing, the shell block parses, the version walk refuses a directory it cannot read or one that holds no manifest, the install is watched before anything else can throw, and a spec runs `bash -n` over every Policy shell block. --- .github/AGENTS.md | 3 +- .github/workflows/pr-check-on-head-branch.yml | 7 ++- scripts/ci-workflow.spec.ts | 29 +++++++-- scripts/pre-merge-check.spec.ts | 32 ++++++++-- scripts/pre-merge-check.ts | 61 +++++++++++++------ scripts/verify-version-sync.ts | 52 ++++++++++++---- 6 files changed, 140 insertions(+), 44 deletions(-) diff --git a/.github/AGENTS.md b/.github/AGENTS.md index e0ff76ca1f..c3e2ee32ee 100644 --- a/.github/AGENTS.md +++ b/.github/AGENTS.md @@ -2,10 +2,9 @@ Run jobs on Windows runners (`windows-*` labels) under native `pwsh`. Native Windows pull-request jobs live in `ci-windows.yml` and Python pull-request jobs in `ci-python.yml`, so each PR workflow can be re-run independently; their checks feed the required `windows checks passed` and `python checks passed` verdicts, as Linux jobs in `ci.yml` feed `all checks passed`; Wine runs Windows Node on hosted Linux only in `ci-master.yml`. Python runtime CI checks Linux/Windows x64 on pull requests and Linux ARM64 plus both macOS architectures on master pushes; releases retain all five targets ([platform policy](workflows/ci-master.yml)). `ci.yml`, `ci-windows.yml`, and `ci-python.yml` are pull-request-only. Master-only platform checks, Linux/Windows self-hosted standbys, and manual runner benchmarks live in `ci-master.yml`, which listens to master pushes and `workflow_dispatch`, not `pull_request`; separating workflow triggers keeps master-only jobs out of PR check panels. The master standbys validate the self-hosted failover targets; preserve the existing per-platform switches (values `selfhosted` for the in-house standbys and `blacksmith` for Blacksmith's hosted runners) and the Dependabot hosted fallback under the `selfhosted` values ([failover runbook](../.agents/notes/implemented/process/2026-07-26-ci-failover-runbook.md), [blacksmith failover leg note](../.agents/notes/implemented/process/2026-09-09-blacksmith-failover-leg.md)). -`Policy / Pre-Merge Check` runs `scripts/pre-merge-check.ts` under plain Node and knows nothing else about the merged tree: that script installs, checks the released manifests carry the root version, and builds the Host and Client contracts, so change it in one pull request together with the build it orchestrates. The manual [Windows packaging workflow](workflows/windows-package.yml) uses the dedicated self-hosted Windows x64 label `dsh-win-package-trial` under native `pwsh`; preserve this routing separately from the automatic CI lanes. -The merge checks run from three workflows named `Policy`, which run on `pull_request_target` so their definitions come from master, and call the `deepseek-harness-check` action directly: `pr-check-on-head-branch.yml` on pull-request updates gives `Policy / Master Change` and `Policy / Pre-Merge Check`, `pr-check-after-approve.yml` on pull-request updates and submitted or dismissed reviews gives `Policy / Approve Check` and `Policy / Reviewer Evaluate`, and `pr-check-on-base-branch.yml` on master pushes re-runs the head-branch checks as `Policy / Recheck`. Branch rules require those check names, so keep the job names and the workflow name. Each check reads its mode from a repository variable: `MERGE_POLICY_MASTER_DIFF`, `MERGE_POLICY_PRE_MERGE`, `MERGE_POLICY_HEAD_DIFF`, and `MERGE_POLICY_REVIEW`, each `off` (skip), `report` (run without failing the job), or `block` (fail the job); a missing value or an unknown one counts as `report`. `Policy / Pre-Merge Check` merges the commit `Policy / Master Change` reports, so it runs only after that job and stays off while `MERGE_POLICY_MASTER_DIFF` is `off`; a merge it cannot do and a build that fails both follow its own mode, so `report` warns and skips the build while `block` fails the job. +The merge checks run from three workflows named `Policy`, which run on `pull_request_target` so their definitions come from master, and call the `deepseek-harness-check` action directly: `pr-check-on-head-branch.yml` on pull-request updates gives `Policy / Master Change` and `Policy / Pre-Merge Check`, `pr-check-after-approve.yml` on pull-request updates and submitted or dismissed reviews gives `Policy / Approve Check` and `Policy / Reviewer Evaluate`, and `pr-check-on-base-branch.yml` on master pushes re-runs the head-branch checks as `Policy / Recheck`. Branch rules require those check names, so keep the job names and the workflow name. Each check reads its mode from a repository variable: `MERGE_POLICY_MASTER_DIFF`, `MERGE_POLICY_PRE_MERGE`, `MERGE_POLICY_HEAD_DIFF`, and `MERGE_POLICY_REVIEW`, each `off` (skip), `report` (run without failing the job), or `block` (fail the job); a missing value or an unknown one counts as `report`. `Policy / Pre-Merge Check` merges the commit `Policy / Master Change` reports, so it runs only after that job and stays off while `MERGE_POLICY_MASTER_DIFF` is `off`; a merge it cannot do and a build that fails both follow its own mode, so `report` warns and skips the build while `block` fails the job. `Policy / Pre-Merge Check` runs `scripts/pre-merge-check.ts` under plain Node and knows nothing else about the merged tree: that script installs, checks that every released manifest carries the root version, and builds the Host and Client contracts, so change it in one pull request together with the build it orchestrates. The manual Desktop publication workflow selects a successful package run and attempt, then routes each retained build through `DESKTOP_PUBLISH_RUNNERS` to its original machine's unique label. Keep Windows run steps on native PowerShell and Mac run steps on the default POSIX shell; shared entrypoints are Node scripts. Test and production jobs use separate `desktop-test` and `desktop-production` approval Environments after a credential-free preparation job. diff --git a/.github/workflows/pr-check-on-head-branch.yml b/.github/workflows/pr-check-on-head-branch.yml index 87298d5534..64ee0b4f19 100644 --- a/.github/workflows/pr-check-on-head-branch.yml +++ b/.github/workflows/pr-check-on-head-branch.yml @@ -100,7 +100,11 @@ jobs: run: | # The check names one file and knows nothing else about how the merged tree builds itself. if [ ! -f scripts/pre-merge-check.ts ]; then - echo "This tree has no scripts/pre-merge-check.ts; the check is skipped." + if [ "$CHECK_MODE" = block ]; then + echo "::error title=Pre-Merge Check::this tree has no scripts/pre-merge-check.ts, so nothing was installed, compiled, or checked" + exit 1 + fi + echo "::warning title=Pre-Merge Check::this tree has no scripts/pre-merge-check.ts, so nothing was installed, compiled, or checked" exit 0 fi if node scripts/pre-merge-check.ts; then @@ -111,4 +115,3 @@ jobs: exit 1 fi echo "::warning title=Pre-Merge Check::the merged tree fails its pre-merge check; this check does not block" - fi diff --git a/scripts/ci-workflow.spec.ts b/scripts/ci-workflow.spec.ts index 3fe2c7c340..8fb4f8dde8 100644 --- a/scripts/ci-workflow.spec.ts +++ b/scripts/ci-workflow.spec.ts @@ -1,3 +1,4 @@ +import { spawnSync } from 'node:child_process' import { readFileSync } from 'node:fs' import { resolve } from 'node:path' import { runInNewContext } from 'node:vm' @@ -1687,6 +1688,25 @@ function isRecord(value: unknown): value is Record { return typeof value === 'object' && value !== null && !Array.isArray(value) } +describe('Policy workflow shells', () => { + it.each(['pr-check-on-head-branch.yml', 'pr-check-after-approve.yml', 'pr-check-on-base-branch.yml'])( + '%s runs shell blocks that parse', + (file) => { + if (process.platform === 'win32') return + const workflow = loadWorkflow('.github/workflows/' + file) + if (!isRecord(workflow.jobs)) throw new TypeError(`${file} must define jobs`) + for (const job of Object.values(workflow.jobs)) { + if (!isRecord(job) || !Array.isArray(job.steps)) continue + for (const step of job.steps) { + if (!isRecord(step) || typeof step.run !== 'string') continue + const parsed = spawnSync('bash', ['-n'], { input: step.run, encoding: 'utf8' }) + expect(`${String(step.name ?? step.id)}: ${parsed.stderr}`).toBe(`${String(step.name ?? step.id)}: `) + } + } + }, + ) +}) + describe('Policy workflows', () => { const workflows = [ ['pr-check-on-head-branch.yml', ['master']], @@ -1726,9 +1746,10 @@ describe('Policy workflows', () => { if (typeof step.run !== 'string') continue expect(step.run).toContain('"$CHECK_MODE" = block') } - for (const step of steps.slice(merge + 1)) { - if (step.name === "Run the tree's pre-merge check") continue - expect(step.if).toBe("steps.merge.outputs.merged == 'true'") - } + for (const step of steps.slice(merge + 1)) expect(step.if).toBe("steps.merge.outputs.merged == 'true'") + const run = steps[build]?.run + if (typeof run !== 'string') throw new TypeError('the pre-merge check step must run a command') + expect(run).toContain('node scripts/pre-merge-check.ts') + expect(run).toContain('"$CHECK_MODE" = block') }) }) diff --git a/scripts/pre-merge-check.spec.ts b/scripts/pre-merge-check.spec.ts index a47fe96d9c..7443596111 100644 --- a/scripts/pre-merge-check.spec.ts +++ b/scripts/pre-merge-check.spec.ts @@ -32,18 +32,18 @@ function plan(codes: readonly number[]): PreMergePlan { describe('pre-merge check', () => { it('passes an install, a version check, and the builds that all succeed', async () => { - const root = repository({ 'package.json': '1.0.0', 'packages/core/session/package.json': '1.0.0' }) + const root = repository({ 'package.json': '1.0.0', 'packages/core/session/package.json': '1.0.0', 'apps/web/package.json': '1.0.0' }) try { const lines: string[] = [] expect(await preMergeCheck(root, { plan: plan([0, 0, 0]), report: line => void lines.push(line) })).toEqual({ ok: true }) - expect(lines[0]).toBe('verify-version-sync: 1 released manifest(s) carry 1.0.0') + expect(lines[0]).toBe('verify-version-sync: 2 released manifest(s) carry 1.0.0') } finally { rmSync(root, { recursive: true, force: true }) } }) it('reports a failed install without running the builds', async () => { - const root = repository({ 'package.json': '1.0.0', 'packages/core/session/package.json': '1.0.0' }) + const root = repository({ 'package.json': '1.0.0', 'packages/core/session/package.json': '1.0.0', 'apps/web/package.json': '1.0.0' }) try { const lines: string[] = [] const outcome = await preMergeCheck(root, { plan: plan([2, 0]), report: line => void lines.push(line) }) @@ -55,7 +55,7 @@ describe('pre-merge check', () => { }) it('fails a tree whose released manifests carry another version', async () => { - const root = repository({ 'package.json': '2.0.0', 'packages/core/session/package.json': '1.0.0' }) + const root = repository({ 'package.json': '2.0.0', 'packages/core/session/package.json': '1.0.0', 'apps/web/package.json': '2.0.0' }) try { const outcome = await preMergeCheck(root, { plan: plan([0, 0, 0]), report: () => {} }) expect(outcome).toEqual({ ok: false, reason: 'the released manifests do not carry the root version' }) @@ -64,6 +64,30 @@ describe('pre-merge check', () => { } }) + it('stops at the first build that fails', async () => { + const root = repository({ 'package.json': '1.0.0', 'packages/core/session/package.json': '1.0.0', 'apps/web/package.json': '1.0.0' }) + try { + const lines: string[] = [] + const outcome = await preMergeCheck(root, { plan: plan([0, 1, 0]), report: line => void lines.push(line) }) + expect(outcome).toEqual({ ok: false, reason: 'the build-0 build failed' }) + expect(lines).toContain('pre-merge-check: building build-0') + expect(lines).not.toContain('pre-merge-check: building build-1') + } finally { + rmSync(root, { recursive: true, force: true }) + } + }) + + it('fails when a manifest directory cannot be read', async () => { + const root = repository({ 'package.json': '1.0.0', 'packages/core/session/package.json': '1.0.0' }) + try { + const outcome = await preMergeCheck(root, { plan: plan([0]), report: () => {} }) + expect(outcome.ok).toBe(false) + expect(outcome.reason).toContain('cannot read the released manifests') + } finally { + rmSync(root, { recursive: true, force: true }) + } + }) + it('builds the host contracts before the client', () => { expect(buildSteps().map(step => step.name)).toEqual(['host contracts', 'client']) }) diff --git a/scripts/pre-merge-check.ts b/scripts/pre-merge-check.ts index 687e9162d7..a68e25eb64 100644 --- a/scripts/pre-merge-check.ts +++ b/scripts/pre-merge-check.ts @@ -2,16 +2,16 @@ * Validate the tree a pull request produces once merged with its base branch: the install, * the version synchronization of the released manifests, and the Host and Client builds. * - * The pull-request check runs this through `pnpm run pre-merge-check` and knows only that - * script's name, so what the merged tree needs changes here without touching the check. The - * script itself runs under plain Node: it spawns pnpm, so no dependency is installed first. + * The `Policy / Pre-Merge Check` job runs this file with `node`, and knows nothing else about the + * merged tree, so what that tree needs changes here without touching the check. The file runs under + * plain Node and spawns pnpm itself, so no dependency is installed before it starts. */ import { spawn, spawnSync } from 'node:child_process' import { resolve } from 'node:path' import process from 'node:process' import { pathToFileURL } from 'node:url' -import { describeVersions, readVersions } from './verify-version-sync.ts' +import { describeVersions, readVersions, versionsSynchronized, type VersionReport } from './verify-version-sync.ts' /** One command of the check. */ export interface PreMergeStep { @@ -44,6 +44,13 @@ export const INSTALL_STEP: PreMergeStep = { name: 'install', command: 'pnpm', ar /** The plan the repository uses. */ export const DEFAULT_PLAN: PreMergePlan = { install: INSTALL_STEP, steps: buildSteps() } +/** How the install ended. */ +interface InstallOutcome { + readonly code: number + /** Why the install could not even start, when it could not. */ + readonly error?: string +} + /** Options of the check. */ export interface PreMergeOptions { /** Sink for the check's own lines. */ @@ -63,11 +70,17 @@ export interface PreMergeOutcome { * Run one step to completion with the caller's terminal, so its output lands in the job log. * @param step - Step to run. * @param root - Working directory. - * @returns Its exit code, or 1 when a signal ended it. + * @param report - Sink for the check's own lines. + * @returns Why the step failed, or `undefined` when it succeeded. */ -function run(step: PreMergeStep, root: string): number { +function run(step: PreMergeStep, root: string, report: (line: string) => void): string | undefined { const result = spawnSync(step.command, [...step.args], { cwd: root, stdio: 'inherit' }) - return result.status ?? 1 + if (result.error !== undefined) { + const reason = `cannot run ${step.command}: ${result.error.message}` + report(`pre-merge-check: ${reason}`) + return reason + } + return result.status === 0 ? undefined : `the ${step.name} build failed` } /** @@ -81,26 +94,34 @@ export async function preMergeCheck(root: string, options: PreMergeOptions = {}) process.stdout.write(`${line}\n`) }) const plan = options.plan ?? DEFAULT_PLAN - // The manifests are read while the install runs: the version check needs no installed dependency. const install = spawn(plan.install.command, [...plan.install.args], { cwd: root, stdio: 'inherit' }) - const versions = readVersions(root) - const lines = describeVersions(versions) - const installCode = await new Promise((settle) => { + // Listen before anything else can throw: a pnpm that cannot start must not leave the check hanging. + const installed = await new Promise((settle) => { install.once('close', (code) => { - settle(code ?? 1) + settle({ code: code ?? 1 }) }) - install.once('error', () => { - settle(1) + install.once('error', (error: Error) => { + settle({ code: 1, error: error.message }) }) - }) - const versionsOk = versions.rootVersion !== undefined && lines.length === 1 - for (const line of lines) report(`verify-version-sync: ${line}`) - if (installCode !== 0) return { ok: false, reason: 'the install failed' } + }).catch((error: unknown) => ({ code: 1, error: error instanceof Error ? error.message : String(error) })) + let versions: VersionReport | undefined + let versionsError: string | undefined + try { + versions = readVersions(root) + } catch (error) { + versionsError = error instanceof Error ? error.message : String(error) + } + // The install runs while the manifests are read, so the version lines appear beside its output. + if (versions !== undefined) for (const line of describeVersions(versions)) report(`verify-version-sync: ${line}`) + if (installed.error !== undefined) return { ok: false, reason: `cannot run ${plan.install.command}: ${installed.error}` } + if (installed.code !== 0) return { ok: false, reason: 'the install failed' } for (const step of plan.steps) { report(`pre-merge-check: building ${step.name}`) - if (run(step, root) !== 0) return { ok: false, reason: `the ${step.name} build failed` } + const failure = run(step, root, report) + if (failure !== undefined) return { ok: false, reason: failure } } - if (!versionsOk) return { ok: false, reason: 'the released manifests do not carry the root version' } + if (versions === undefined) return { ok: false, reason: `cannot read the released manifests: ${versionsError ?? 'unknown error'}` } + if (!versionsSynchronized(versions)) return { ok: false, reason: 'the released manifests do not carry the root version' } return { ok: true } } diff --git a/scripts/verify-version-sync.ts b/scripts/verify-version-sync.ts index b94ae39dac..a194665e64 100644 --- a/scripts/verify-version-sync.ts +++ b/scripts/verify-version-sync.ts @@ -28,8 +28,14 @@ export interface VersionReport { /** Directories that hold released manifests below the repository root. */ const MANIFEST_DIRECTORIES = ['packages', 'apps'] as const -/** Directories whose manifests follow another version line. */ -const EXCLUDED_DIRECTORIES = ['vendor', 'native'] as const +/** + * Describe an error for a reader. + * @param error - Caught value. + * @returns Its message. + */ +function messageOf(error: unknown): string { + return error instanceof Error ? error.message : String(error) +} /** * Read the version one `package.json` declares. @@ -44,8 +50,9 @@ function versionOf(path: string): string | undefined { } /** - * Collect the manifests that must carry the root version: two levels under each - * workspace directory, so `packages//` and `apps/`. + * Collect the manifests that must carry the root version: `packages//` and + * `apps/`. A directory that cannot be read, and a walk that finds nothing, are + * errors rather than a clean tree, so a narrowed corpus cannot pass the check. * @param root - Repository root. * @returns Manifest paths relative to the root, sorted. */ @@ -55,13 +62,18 @@ export function releasedManifests(root: string): string[] { let groups: string[] try { groups = readdirSync(join(root, directory)) - } catch { - continue + } catch (error) { + throw new Error(`cannot read ${directory}/: ${messageOf(error)}`) } for (const group of groups) { - if ((EXCLUDED_DIRECTORIES as readonly string[]).includes(group)) continue const groupDirectory = join(root, directory, group) - if (!statSync(groupDirectory).isDirectory()) continue + let isDirectory: boolean + try { + isDirectory = statSync(groupDirectory).isDirectory() + } catch (error) { + throw new Error(`cannot read ${directory}/${group}: ${messageOf(error)}`) + } + if (!isDirectory) continue const manifest = join(groupDirectory, 'package.json') try { if (statSync(manifest).isFile()) paths.push(`${directory}/${group}/package.json`) @@ -78,6 +90,7 @@ export function releasedManifests(root: string): string[] { } } } + if (paths.length === 0) throw new Error(`no released manifest was found under ${MANIFEST_DIRECTORIES.map(directory => `${directory}/`).join(' or ')}`) return paths.sort() } @@ -102,6 +115,16 @@ export function readVersions(root: string): VersionReport { return { rootVersion, manifests } } +/** + * Report whether every released manifest carries the root version. + * @param report - Root version and manifests. + * @returns Whether the tree is synchronized; a tree without a root version or without manifests is not. + */ +export function versionsSynchronized(report: VersionReport): boolean { + const { rootVersion, manifests } = report + return rootVersion !== undefined && manifests.length > 0 && mismatchedVersions(rootVersion, manifests).length === 0 +} + /** * Describe a version mismatch for a reader. * @param report - Root version and manifests. @@ -121,8 +144,13 @@ export function describeVersions(report: VersionReport): string[] { const invokedPath = process.argv[1] const isMain = invokedPath !== undefined && import.meta.url === pathToFileURL(resolve(invokedPath)).href if (isMain) { - const lines = describeVersions(readVersions(resolve(import.meta.dirname, '..'))) - const failed = lines.length > 1 || lines[0]?.includes('declares no version') === true - for (const line of lines) (failed ? console.error : console.log)(`verify-version-sync: ${line}`) - if (failed) process.exitCode = 1 + try { + const report = readVersions(resolve(import.meta.dirname, '..')) + const synchronized = versionsSynchronized(report) + for (const line of describeVersions(report)) (synchronized ? console.log : console.error)(`verify-version-sync: ${line}`) + if (!synchronized) process.exitCode = 1 + } catch (error) { + console.error(`verify-version-sync: ${messageOf(error)}`) + process.exitCode = 1 + } } From 1e433706f4e03c002f1e248afeb1a16f74da835e Mon Sep 17 00:00:00 2001 From: imccyu Date: Mon, 26 Oct 2026 23:24:20 +0800 Subject: [PATCH 3/6] refactor(ci): run the pre-merge steps as one ordered list Each step is a spawn whose stdio reaches the job log, the first failure stops the list, and the script decides from CHECK_MODE whether to annotate as an error or a warning, so the workflow only calls it. --- .github/workflows/pr-check-on-head-branch.yml | 9 +- scripts/ci-workflow.spec.ts | 3 +- scripts/pre-merge-check.spec.ts | 105 ++++++++++----- scripts/pre-merge-check.ts | 124 +++++++++--------- 4 files changed, 136 insertions(+), 105 deletions(-) diff --git a/.github/workflows/pr-check-on-head-branch.yml b/.github/workflows/pr-check-on-head-branch.yml index 64ee0b4f19..ae9186e1e9 100644 --- a/.github/workflows/pr-check-on-head-branch.yml +++ b/.github/workflows/pr-check-on-head-branch.yml @@ -107,11 +107,4 @@ jobs: echo "::warning title=Pre-Merge Check::this tree has no scripts/pre-merge-check.ts, so nothing was installed, compiled, or checked" exit 0 fi - if node scripts/pre-merge-check.ts; then - exit 0 - fi - if [ "$CHECK_MODE" = block ]; then - echo "::error title=Pre-Merge Check::the merged tree fails its pre-merge check" - exit 1 - fi - echo "::warning title=Pre-Merge Check::the merged tree fails its pre-merge check; this check does not block" + exec node scripts/pre-merge-check.ts diff --git a/scripts/ci-workflow.spec.ts b/scripts/ci-workflow.spec.ts index 8fb4f8dde8..4ab2de4263 100644 --- a/scripts/ci-workflow.spec.ts +++ b/scripts/ci-workflow.spec.ts @@ -1750,6 +1750,7 @@ describe('Policy workflows', () => { const run = steps[build]?.run if (typeof run !== 'string') throw new TypeError('the pre-merge check step must run a command') expect(run).toContain('node scripts/pre-merge-check.ts') - expect(run).toContain('"$CHECK_MODE" = block') + // The script annotates its own failure, so the step decides by its exit code alone. + expect(run).not.toContain('the merged tree fails its pre-merge check') }) }) diff --git a/scripts/pre-merge-check.spec.ts b/scripts/pre-merge-check.spec.ts index 7443596111..79a7536cc2 100644 --- a/scripts/pre-merge-check.spec.ts +++ b/scripts/pre-merge-check.spec.ts @@ -2,15 +2,17 @@ import { mkdirSync, mkdtempSync, rmSync, writeFileSync } from 'node:fs' import { tmpdir } from 'node:os' import { dirname, join } from 'node:path' import { describe, expect, it } from 'vitest' -import { buildSteps, preMergeCheck, type PreMergePlan } from './pre-merge-check.ts' +import { checkMode, failureDecision, preMergeCheck, preMergeSteps, type PreMergeOutcome, type PreMergeStep } from './pre-merge-check.ts' /** * Build a repository holding the given manifests. * @param files - Manifest path to version. + * @param directories - Extra directories to create. * @returns The temporary root. */ -function repository(files: Record): string { +function repository(files: Record, directories: readonly string[] = []): string { const root = mkdtempSync(join(tmpdir(), 'dsh-pre-merge-')) + for (const directory of directories) mkdirSync(join(root, directory), { recursive: true }) for (const [path, version] of Object.entries(files)) { const absolute = join(root, path) mkdirSync(dirname(absolute), { recursive: true }) @@ -20,35 +22,65 @@ function repository(files: Record): string { } /** - * A plan whose commands exit with the given codes, so the check runs without pnpm. - * @param codes - Exit code of the install and of each build. - * @returns The plan. + * Steps that exit with the given codes, so the check runs without pnpm. + * @param codes - Exit code of each step, in order. + * @returns The steps. */ -function plan(codes: readonly number[]): PreMergePlan { - const step = (name: string, code: number) => ({ name, command: process.execPath, args: ['-e', `process.exit(${code})`] }) - const [install, ...builds] = codes - return { install: step('install', install ?? 0), steps: builds.map((code, index) => step(`build-${index}`, code)) } +function steps(codes: readonly number[]): PreMergeStep[] { + return codes.map((code, index) => ({ name: `step-${index}`, command: process.execPath, args: ['-e', `process.exit(${code})`] })) +} + +/** What one run reported. */ +interface CheckRun { + readonly outcome: PreMergeOutcome + readonly lines: string[] +} + +/** + * Run the check on a repository and collect its lines. + * @param root - Repository root. + * @param codes - Exit code of each step. + * @returns Outcome and reported lines. + */ +async function check(root: string, codes: readonly number[]): Promise { + const lines: string[] = [] + const outcome = await preMergeCheck(root, { steps: steps(codes), report: line => void lines.push(line) }) + return { outcome, lines } } describe('pre-merge check', () => { - it('passes an install, a version check, and the builds that all succeed', async () => { + it('passes the steps and the version check when they all succeed', async () => { const root = repository({ 'package.json': '1.0.0', 'packages/core/session/package.json': '1.0.0', 'apps/web/package.json': '1.0.0' }) try { - const lines: string[] = [] - expect(await preMergeCheck(root, { plan: plan([0, 0, 0]), report: line => void lines.push(line) })).toEqual({ ok: true }) + const { outcome, lines } = await check(root, [0, 0, 0]) + expect(outcome).toEqual({ ok: true }) expect(lines[0]).toBe('verify-version-sync: 2 released manifest(s) carry 1.0.0') + expect(lines.slice(1)).toEqual(['pre-merge-check: running step-0', 'pre-merge-check: running step-1', 'pre-merge-check: running step-2']) } finally { rmSync(root, { recursive: true, force: true }) } }) - it('reports a failed install without running the builds', async () => { + it('stops at the first step that fails', async () => { const root = repository({ 'package.json': '1.0.0', 'packages/core/session/package.json': '1.0.0', 'apps/web/package.json': '1.0.0' }) try { - const lines: string[] = [] - const outcome = await preMergeCheck(root, { plan: plan([2, 0]), report: line => void lines.push(line) }) - expect(outcome).toEqual({ ok: false, reason: 'the install failed' }) - expect(lines.some(line => line.startsWith('pre-merge-check: building'))).toBe(false) + const { outcome, lines } = await check(root, [1, 0]) + expect(outcome).toEqual({ ok: false, reason: 'the step-0 step failed' }) + expect(lines).not.toContain('pre-merge-check: running step-1') + } finally { + rmSync(root, { recursive: true, force: true }) + } + }) + + it('reports a command that cannot start', async () => { + const root = repository({ 'package.json': '1.0.0', 'packages/core/session/package.json': '1.0.0', 'apps/web/package.json': '1.0.0' }) + try { + const outcome = await preMergeCheck(root, { + steps: [{ name: 'missing', command: join(root, 'no-such-command'), args: [] }], + report: () => {}, + }) + expect(outcome.ok).toBe(false) + expect(outcome.reason).toContain('cannot run') } finally { rmSync(root, { recursive: true, force: true }) } @@ -57,38 +89,43 @@ describe('pre-merge check', () => { it('fails a tree whose released manifests carry another version', async () => { const root = repository({ 'package.json': '2.0.0', 'packages/core/session/package.json': '1.0.0', 'apps/web/package.json': '2.0.0' }) try { - const outcome = await preMergeCheck(root, { plan: plan([0, 0, 0]), report: () => {} }) + const { outcome } = await check(root, [0, 0]) expect(outcome).toEqual({ ok: false, reason: 'the released manifests do not carry the root version' }) } finally { rmSync(root, { recursive: true, force: true }) } }) - it('stops at the first build that fails', async () => { - const root = repository({ 'package.json': '1.0.0', 'packages/core/session/package.json': '1.0.0', 'apps/web/package.json': '1.0.0' }) - try { - const lines: string[] = [] - const outcome = await preMergeCheck(root, { plan: plan([0, 1, 0]), report: line => void lines.push(line) }) - expect(outcome).toEqual({ ok: false, reason: 'the build-0 build failed' }) - expect(lines).toContain('pre-merge-check: building build-0') - expect(lines).not.toContain('pre-merge-check: building build-1') - } finally { - rmSync(root, { recursive: true, force: true }) - } - }) - it('fails when a manifest directory cannot be read', async () => { const root = repository({ 'package.json': '1.0.0', 'packages/core/session/package.json': '1.0.0' }) try { - const outcome = await preMergeCheck(root, { plan: plan([0]), report: () => {} }) + const { outcome, lines } = await check(root, [0]) expect(outcome.ok).toBe(false) expect(outcome.reason).toContain('cannot read the released manifests') + expect(lines[0]).toContain('cannot read the released manifests') } finally { rmSync(root, { recursive: true, force: true }) } }) - it('builds the host contracts before the client', () => { - expect(buildSteps().map(step => step.name)).toEqual(['host contracts', 'client']) + it('annotates a failure as a warning and passes in report mode', () => { + expect(checkMode('report')).toBe('report') + expect(checkMode('block')).toBe('block') + expect(checkMode('off')).toBe('off') + expect(checkMode('nonsense')).toBe('report') + expect(checkMode(undefined)).toBe('report') + expect(failureDecision('report', 'the install step failed')).toEqual({ + line: '::warning title=Pre-Merge Check::the merged tree fails its pre-merge check: the install step failed', + exitCode: 0, + }) + expect(failureDecision('block', 'the install step failed')).toEqual({ + line: '::error title=Pre-Merge Check::the merged tree fails its pre-merge check: the install step failed', + exitCode: 1, + }) + expect(failureDecision('off', 'the client step failed').exitCode).toBe(0) + }) + + it('installs before it builds the host contracts and the client', () => { + expect(preMergeSteps().map(step => step.name)).toEqual(['install', 'host contracts', 'client']) }) }) diff --git a/scripts/pre-merge-check.ts b/scripts/pre-merge-check.ts index a68e25eb64..697be8b5aa 100644 --- a/scripts/pre-merge-check.ts +++ b/scripts/pre-merge-check.ts @@ -1,13 +1,13 @@ /** - * Validate the tree a pull request produces once merged with its base branch: the install, - * the version synchronization of the released manifests, and the Host and Client builds. + * Validate the tree a pull request produces once merged with its base branch: the install, the + * version synchronization of the released manifests, and the Host and Client builds. * * The `Policy / Pre-Merge Check` job runs this file with `node`, and knows nothing else about the * merged tree, so what that tree needs changes here without touching the check. The file runs under * plain Node and spawns pnpm itself, so no dependency is installed before it starts. */ -import { spawn, spawnSync } from 'node:child_process' +import { spawn } from 'node:child_process' import { resolve } from 'node:path' import process from 'node:process' import { pathToFileURL } from 'node:url' @@ -21,42 +21,52 @@ export interface PreMergeStep { readonly args: readonly string[] } -/** What the check runs: the install every other step depends on, then the builds in order. */ -export interface PreMergePlan { - readonly install: PreMergeStep - readonly steps: readonly PreMergeStep[] -} - -/** - * The commands that build the merged tree, in order. - * @returns Steps after the install. - */ -export function buildSteps(): PreMergeStep[] { +/** No step can run before the others without the install, so it opens the list. */ +export function preMergeSteps(): PreMergeStep[] { return [ + { name: 'install', command: 'pnpm', args: ['install', '--frozen-lockfile'] }, { name: 'host contracts', command: 'pnpm', args: ['exec', 'tsx', 'scripts/compile-referenced-projects.ts', 'host-contracts'] }, { name: 'client', command: 'pnpm', args: ['exec', 'tsx', 'scripts/compile-referenced-projects.ts', 'client'] }, ] } -/** The install every other step depends on. */ -export const INSTALL_STEP: PreMergeStep = { name: 'install', command: 'pnpm', args: ['install', '--frozen-lockfile'] } - -/** The plan the repository uses. */ -export const DEFAULT_PLAN: PreMergePlan = { install: INSTALL_STEP, steps: buildSteps() } - -/** How the install ended. */ -interface InstallOutcome { - readonly code: number - /** Why the install could not even start, when it could not. */ - readonly error?: string -} - /** Options of the check. */ export interface PreMergeOptions { /** Sink for the check's own lines. */ readonly report?: (line: string) => void - /** Plan to run; the repository's own plan when absent. */ - readonly plan?: PreMergePlan + /** Steps to run; the repository's own steps when absent. */ + readonly steps?: readonly PreMergeStep[] +} + +/** How the job that runs this file reacts to a failure. */ +export type CheckMode = 'off' | 'report' | 'block' + +/** What a failure prints and the exit code it leaves. */ +export interface FailureDecision { + readonly line: string + readonly exitCode: number +} + +/** + * Read the job's mode. The check owns its own mode handling, so the workflow only calls it. + * @param value - The `CHECK_MODE` value, when the environment provides one. + * @returns The mode; a missing or unknown value reports instead of blocking. + */ +export function checkMode(value: string | undefined = process.env.CHECK_MODE): CheckMode { + return value === 'off' || value === 'block' ? value : 'report' +} + +/** + * Decide how a failed tree is reported, so `report` annotates and passes while `block` fails the job. + * @param mode - Mode of the job. + * @param reason - Why the tree failed. + * @returns The annotation and exit code for that mode. + */ +export function failureDecision(mode: CheckMode, reason: string): FailureDecision { + const summary = `the merged tree fails its pre-merge check: ${reason}` + if (mode === 'block') return { line: `::error title=Pre-Merge Check::${summary}`, exitCode: 1 } + if (mode === 'report') return { line: `::warning title=Pre-Merge Check::${summary}`, exitCode: 0 } + return { line: `pre-merge-check: ${summary}`, exitCode: 0 } } /** What the check decided. */ @@ -67,60 +77,49 @@ export interface PreMergeOutcome { } /** - * Run one step to completion with the caller's terminal, so its output lands in the job log. + * Run one step with the caller's terminal, so its output reaches the job log as it arrives. * @param step - Step to run. * @param root - Working directory. - * @param report - Sink for the check's own lines. * @returns Why the step failed, or `undefined` when it succeeded. */ -function run(step: PreMergeStep, root: string, report: (line: string) => void): string | undefined { - const result = spawnSync(step.command, [...step.args], { cwd: root, stdio: 'inherit' }) - if (result.error !== undefined) { - const reason = `cannot run ${step.command}: ${result.error.message}` - report(`pre-merge-check: ${reason}`) - return reason - } - return result.status === 0 ? undefined : `the ${step.name} build failed` +function run(step: PreMergeStep, root: string): Promise { + return new Promise((settle) => { + const child = spawn(step.command, [...step.args], { cwd: root, stdio: 'inherit' }) + child.once('error', (error: Error) => { + settle(`cannot run ${step.command}: ${error.message}`) + }) + child.once('close', (code) => { + settle(code === 0 ? undefined : `the ${step.name} step failed`) + }) + }) } /** * Check the merged tree, reporting why it failed. * @param root - Repository root. - * @param options - Sink for the check's own lines, and the plan to run. + * @param options - Sink for the check's own lines, and the steps to run. * @returns Whether the tree passed, with the reason when it did not. */ export async function preMergeCheck(root: string, options: PreMergeOptions = {}): Promise { const report = options.report ?? ((line: string): void => { process.stdout.write(`${line}\n`) }) - const plan = options.plan ?? DEFAULT_PLAN - const install = spawn(plan.install.command, [...plan.install.args], { cwd: root, stdio: 'inherit' }) - // Listen before anything else can throw: a pnpm that cannot start must not leave the check hanging. - const installed = await new Promise((settle) => { - install.once('close', (code) => { - settle({ code: code ?? 1 }) - }) - install.once('error', (error: Error) => { - settle({ code: 1, error: error.message }) - }) - }).catch((error: unknown) => ({ code: 1, error: error instanceof Error ? error.message : String(error) })) + // The manifests are read before the steps, so the version check needs no installed dependency. let versions: VersionReport | undefined - let versionsError: string | undefined + let versionsError = 'unknown error' try { versions = readVersions(root) + for (const line of describeVersions(versions)) report(`verify-version-sync: ${line}`) } catch (error) { versionsError = error instanceof Error ? error.message : String(error) + report(`verify-version-sync: cannot read the released manifests: ${versionsError}`) } - // The install runs while the manifests are read, so the version lines appear beside its output. - if (versions !== undefined) for (const line of describeVersions(versions)) report(`verify-version-sync: ${line}`) - if (installed.error !== undefined) return { ok: false, reason: `cannot run ${plan.install.command}: ${installed.error}` } - if (installed.code !== 0) return { ok: false, reason: 'the install failed' } - for (const step of plan.steps) { - report(`pre-merge-check: building ${step.name}`) - const failure = run(step, root, report) + for (const step of options.steps ?? preMergeSteps()) { + report(`pre-merge-check: running ${step.name}`) + const failure = await run(step, root) if (failure !== undefined) return { ok: false, reason: failure } } - if (versions === undefined) return { ok: false, reason: `cannot read the released manifests: ${versionsError ?? 'unknown error'}` } + if (versions === undefined) return { ok: false, reason: `cannot read the released manifests: ${versionsError}` } if (!versionsSynchronized(versions)) return { ok: false, reason: 'the released manifests do not carry the root version' } return { ok: true } } @@ -132,7 +131,8 @@ if (isMain) { if (outcome.ok) { process.stdout.write('pre-merge-check: passed\n') } else { - process.stderr.write(`pre-merge-check: failed: ${outcome.reason ?? 'unknown reason'}\n`) - process.exitCode = 1 + const decision = failureDecision(checkMode(), outcome.reason ?? 'unknown reason') + ;(decision.exitCode === 0 ? process.stdout : process.stderr).write(`${decision.line}\n`) + process.exitCode = decision.exitCode } } From 2b485ea1db53084e7e3ff3169d0874abcc06cc53 Mon Sep 17 00:00:00 2001 From: imccyu Date: Mon, 26 Oct 2026 23:27:39 +0800 Subject: [PATCH 4/6] fix(ci): run the version check as the first step of the pre-merge check It reads the manifests without an install, so it opens the step list and a mistyped release stops the check before anything is built. --- scripts/pre-merge-check.spec.ts | 96 ++++++--------------------------- scripts/pre-merge-check.ts | 51 ++++++++---------- 2 files changed, 37 insertions(+), 110 deletions(-) diff --git a/scripts/pre-merge-check.spec.ts b/scripts/pre-merge-check.spec.ts index 79a7536cc2..6027d0c60a 100644 --- a/scripts/pre-merge-check.spec.ts +++ b/scripts/pre-merge-check.spec.ts @@ -1,25 +1,8 @@ -import { mkdirSync, mkdtempSync, rmSync, writeFileSync } from 'node:fs' +import { mkdtempSync, rmSync } from 'node:fs' import { tmpdir } from 'node:os' -import { dirname, join } from 'node:path' +import { join } from 'node:path' import { describe, expect, it } from 'vitest' -import { checkMode, failureDecision, preMergeCheck, preMergeSteps, type PreMergeOutcome, type PreMergeStep } from './pre-merge-check.ts' - -/** - * Build a repository holding the given manifests. - * @param files - Manifest path to version. - * @param directories - Extra directories to create. - * @returns The temporary root. - */ -function repository(files: Record, directories: readonly string[] = []): string { - const root = mkdtempSync(join(tmpdir(), 'dsh-pre-merge-')) - for (const directory of directories) mkdirSync(join(root, directory), { recursive: true }) - for (const [path, version] of Object.entries(files)) { - const absolute = join(root, path) - mkdirSync(dirname(absolute), { recursive: true }) - writeFileSync(absolute, JSON.stringify({ name: 'x', version })) - } - return root -} +import { checkMode, failureDecision, preMergeCheck, preMergeSteps, type PreMergeStep } from './pre-merge-check.ts' /** * Steps that exit with the given codes, so the check runs without pnpm. @@ -30,50 +13,23 @@ function steps(codes: readonly number[]): PreMergeStep[] { return codes.map((code, index) => ({ name: `step-${index}`, command: process.execPath, args: ['-e', `process.exit(${code})`] })) } -/** What one run reported. */ -interface CheckRun { - readonly outcome: PreMergeOutcome - readonly lines: string[] -} - -/** - * Run the check on a repository and collect its lines. - * @param root - Repository root. - * @param codes - Exit code of each step. - * @returns Outcome and reported lines. - */ -async function check(root: string, codes: readonly number[]): Promise { - const lines: string[] = [] - const outcome = await preMergeCheck(root, { steps: steps(codes), report: line => void lines.push(line) }) - return { outcome, lines } -} - describe('pre-merge check', () => { - it('passes the steps and the version check when they all succeed', async () => { - const root = repository({ 'package.json': '1.0.0', 'packages/core/session/package.json': '1.0.0', 'apps/web/package.json': '1.0.0' }) - try { - const { outcome, lines } = await check(root, [0, 0, 0]) - expect(outcome).toEqual({ ok: true }) - expect(lines[0]).toBe('verify-version-sync: 2 released manifest(s) carry 1.0.0') - expect(lines.slice(1)).toEqual(['pre-merge-check: running step-0', 'pre-merge-check: running step-1', 'pre-merge-check: running step-2']) - } finally { - rmSync(root, { recursive: true, force: true }) - } + it('runs every step in order and passes when they all succeed', async () => { + const lines: string[] = [] + const outcome = await preMergeCheck(process.cwd(), { steps: steps([0, 0, 0]), report: line => void lines.push(line) }) + expect(outcome).toEqual({ ok: true }) + expect(lines).toEqual(['pre-merge-check: running step-0', 'pre-merge-check: running step-1', 'pre-merge-check: running step-2']) }) it('stops at the first step that fails', async () => { - const root = repository({ 'package.json': '1.0.0', 'packages/core/session/package.json': '1.0.0', 'apps/web/package.json': '1.0.0' }) - try { - const { outcome, lines } = await check(root, [1, 0]) - expect(outcome).toEqual({ ok: false, reason: 'the step-0 step failed' }) - expect(lines).not.toContain('pre-merge-check: running step-1') - } finally { - rmSync(root, { recursive: true, force: true }) - } + const lines: string[] = [] + const outcome = await preMergeCheck(process.cwd(), { steps: steps([1, 0]), report: line => void lines.push(line) }) + expect(outcome).toEqual({ ok: false, reason: 'the step-0 step failed' }) + expect(lines).not.toContain('pre-merge-check: running step-1') }) it('reports a command that cannot start', async () => { - const root = repository({ 'package.json': '1.0.0', 'packages/core/session/package.json': '1.0.0', 'apps/web/package.json': '1.0.0' }) + const root = mkdtempSync(join(tmpdir(), 'dsh-pre-merge-')) try { const outcome = await preMergeCheck(root, { steps: [{ name: 'missing', command: join(root, 'no-such-command'), args: [] }], @@ -86,26 +42,8 @@ describe('pre-merge check', () => { } }) - it('fails a tree whose released manifests carry another version', async () => { - const root = repository({ 'package.json': '2.0.0', 'packages/core/session/package.json': '1.0.0', 'apps/web/package.json': '2.0.0' }) - try { - const { outcome } = await check(root, [0, 0]) - expect(outcome).toEqual({ ok: false, reason: 'the released manifests do not carry the root version' }) - } finally { - rmSync(root, { recursive: true, force: true }) - } - }) - - it('fails when a manifest directory cannot be read', async () => { - const root = repository({ 'package.json': '1.0.0', 'packages/core/session/package.json': '1.0.0' }) - try { - const { outcome, lines } = await check(root, [0]) - expect(outcome.ok).toBe(false) - expect(outcome.reason).toContain('cannot read the released manifests') - expect(lines[0]).toContain('cannot read the released manifests') - } finally { - rmSync(root, { recursive: true, force: true }) - } + it('checks the versions before it installs, and installs before the builds', () => { + expect(preMergeSteps('/repo').map(step => step.name)).toEqual(['versions', 'install', 'host contracts', 'client']) }) it('annotates a failure as a warning and passes in report mode', () => { @@ -124,8 +62,4 @@ describe('pre-merge check', () => { }) expect(failureDecision('off', 'the client step failed').exitCode).toBe(0) }) - - it('installs before it builds the host contracts and the client', () => { - expect(preMergeSteps().map(step => step.name)).toEqual(['install', 'host contracts', 'client']) - }) }) diff --git a/scripts/pre-merge-check.ts b/scripts/pre-merge-check.ts index 697be8b5aa..832cac8735 100644 --- a/scripts/pre-merge-check.ts +++ b/scripts/pre-merge-check.ts @@ -1,17 +1,16 @@ /** - * Validate the tree a pull request produces once merged with its base branch: the install, the - * version synchronization of the released manifests, and the Host and Client builds. + * Validate the tree a pull request produces once merged with its base branch: the version + * synchronization of the released manifests, the install, and the Host and Client builds. * * The `Policy / Pre-Merge Check` job runs this file with `node`, and knows nothing else about the * merged tree, so what that tree needs changes here without touching the check. The file runs under - * plain Node and spawns pnpm itself, so no dependency is installed before it starts. + * plain Node and spawns its steps itself, so no dependency is installed before it starts. */ import { spawn } from 'node:child_process' -import { resolve } from 'node:path' +import { join, resolve } from 'node:path' import process from 'node:process' import { pathToFileURL } from 'node:url' -import { describeVersions, readVersions, versionsSynchronized, type VersionReport } from './verify-version-sync.ts' /** One command of the check. */ export interface PreMergeStep { @@ -21,23 +20,21 @@ export interface PreMergeStep { readonly args: readonly string[] } -/** No step can run before the others without the install, so it opens the list. */ -export function preMergeSteps(): PreMergeStep[] { +/** + * The steps that check the merged tree, in order. The versions come first because reading the + * manifests needs no install, so a mistyped release stops the check before anything is built. + * @param root - Repository root. + * @returns Steps in run order. + */ +export function preMergeSteps(root: string): PreMergeStep[] { return [ + { name: 'versions', command: process.execPath, args: [join(root, 'scripts', 'verify-version-sync.ts')] }, { name: 'install', command: 'pnpm', args: ['install', '--frozen-lockfile'] }, { name: 'host contracts', command: 'pnpm', args: ['exec', 'tsx', 'scripts/compile-referenced-projects.ts', 'host-contracts'] }, { name: 'client', command: 'pnpm', args: ['exec', 'tsx', 'scripts/compile-referenced-projects.ts', 'client'] }, ] } -/** Options of the check. */ -export interface PreMergeOptions { - /** Sink for the check's own lines. */ - readonly report?: (line: string) => void - /** Steps to run; the repository's own steps when absent. */ - readonly steps?: readonly PreMergeStep[] -} - /** How the job that runs this file reacts to a failure. */ export type CheckMode = 'off' | 'report' | 'block' @@ -69,6 +66,14 @@ export function failureDecision(mode: CheckMode, reason: string): FailureDecisio return { line: `pre-merge-check: ${summary}`, exitCode: 0 } } +/** Options of the check. */ +export interface PreMergeOptions { + /** Sink for the check's own lines. */ + readonly report?: (line: string) => void + /** Steps to run; the repository's own steps when absent. */ + readonly steps?: readonly PreMergeStep[] +} + /** What the check decided. */ export interface PreMergeOutcome { readonly ok: boolean @@ -95,7 +100,7 @@ function run(step: PreMergeStep, root: string): Promise { } /** - * Check the merged tree, reporting why it failed. + * Run every step of the merged tree in order, stopping at the first failure. * @param root - Repository root. * @param options - Sink for the check's own lines, and the steps to run. * @returns Whether the tree passed, with the reason when it did not. @@ -104,23 +109,11 @@ export async function preMergeCheck(root: string, options: PreMergeOptions = {}) const report = options.report ?? ((line: string): void => { process.stdout.write(`${line}\n`) }) - // The manifests are read before the steps, so the version check needs no installed dependency. - let versions: VersionReport | undefined - let versionsError = 'unknown error' - try { - versions = readVersions(root) - for (const line of describeVersions(versions)) report(`verify-version-sync: ${line}`) - } catch (error) { - versionsError = error instanceof Error ? error.message : String(error) - report(`verify-version-sync: cannot read the released manifests: ${versionsError}`) - } - for (const step of options.steps ?? preMergeSteps()) { + for (const step of options.steps ?? preMergeSteps(root)) { report(`pre-merge-check: running ${step.name}`) const failure = await run(step, root) if (failure !== undefined) return { ok: false, reason: failure } } - if (versions === undefined) return { ok: false, reason: `cannot read the released manifests: ${versionsError}` } - if (!versionsSynchronized(versions)) return { ok: false, reason: 'the released manifests do not carry the root version' } return { ok: true } } From 3ddacfd725ef4421dd530459ce74c0765ff8187b Mon Sep 17 00:00:00 2001 From: imccyu Date: Mon, 26 Oct 2026 23:51:18 +0800 Subject: [PATCH 5/6] fix(ci): narrow the version scan and keep a crashing check inside its mode The scan now reads exactly the manifests the release tooling rewrites, names every directory and file it cannot read, and refuses an empty corpus; the command takes the root it checks so the exit code is tested, and the workflow reports a crashed check as a warning in report mode. --- .github/workflows/pr-check-on-head-branch.yml | 10 +- scripts/ci-workflow.spec.ts | 7 +- scripts/pre-merge-check.spec.ts | 7 +- scripts/pre-merge-check.ts | 6 +- scripts/verify-version-sync.spec.ts | 89 ++++++++-- scripts/verify-version-sync.ts | 158 ++++++++++++------ 6 files changed, 202 insertions(+), 75 deletions(-) diff --git a/.github/workflows/pr-check-on-head-branch.yml b/.github/workflows/pr-check-on-head-branch.yml index ae9186e1e9..62d762d72f 100644 --- a/.github/workflows/pr-check-on-head-branch.yml +++ b/.github/workflows/pr-check-on-head-branch.yml @@ -107,4 +107,12 @@ jobs: echo "::warning title=Pre-Merge Check::this tree has no scripts/pre-merge-check.ts, so nothing was installed, compiled, or checked" exit 0 fi - exec node scripts/pre-merge-check.ts + if node scripts/pre-merge-check.ts; then + exit 0 + fi + # A check that crashes reports nothing, so the step reports its exit the way the mode asks. + if [ "$CHECK_MODE" = block ]; then + echo "::error title=Pre-Merge Check::scripts/pre-merge-check.ts exited without reporting, so the merged tree is unchecked" + exit 1 + fi + echo "::warning title=Pre-Merge Check::scripts/pre-merge-check.ts exited without reporting, so the merged tree is unchecked" diff --git a/scripts/ci-workflow.spec.ts b/scripts/ci-workflow.spec.ts index 4ab2de4263..c3ac5f299b 100644 --- a/scripts/ci-workflow.spec.ts +++ b/scripts/ci-workflow.spec.ts @@ -1700,7 +1700,7 @@ describe('Policy workflow shells', () => { for (const step of job.steps) { if (!isRecord(step) || typeof step.run !== 'string') continue const parsed = spawnSync('bash', ['-n'], { input: step.run, encoding: 'utf8' }) - expect(`${String(step.name ?? step.id)}: ${parsed.stderr}`).toBe(`${String(step.name ?? step.id)}: `) + expect(`${String(step.name ?? step.id)}: ${parsed.status}: ${parsed.stderr}`).toBe(`${String(step.name ?? step.id)}: 0: `) } } }, @@ -1750,7 +1750,8 @@ describe('Policy workflows', () => { const run = steps[build]?.run if (typeof run !== 'string') throw new TypeError('the pre-merge check step must run a command') expect(run).toContain('node scripts/pre-merge-check.ts') - // The script annotates its own failure, so the step decides by its exit code alone. - expect(run).not.toContain('the merged tree fails its pre-merge check') + // The script annotates its own failures; the step keeps a mode-aware guard for a check that crashes. + expect(run).toContain('exited without reporting') + expect(run).toContain('"$CHECK_MODE" = block') }) }) diff --git a/scripts/pre-merge-check.spec.ts b/scripts/pre-merge-check.spec.ts index 6027d0c60a..fb49b9bc3b 100644 --- a/scripts/pre-merge-check.spec.ts +++ b/scripts/pre-merge-check.spec.ts @@ -43,7 +43,11 @@ describe('pre-merge check', () => { }) it('checks the versions before it installs, and installs before the builds', () => { - expect(preMergeSteps('/repo').map(step => step.name)).toEqual(['versions', 'install', 'host contracts', 'client']) + const steps = preMergeSteps('/repo') + expect(steps.map(step => step.name)).toEqual(['versions', 'install', 'host contracts', 'client']) + expect(steps[0]).toEqual({ name: 'versions', command: process.execPath, args: ['/repo/scripts/verify-version-sync.ts', '/repo'] }) + expect(steps[1]).toEqual({ name: 'install', command: 'pnpm', args: ['install', '--frozen-lockfile'] }) + expect(steps[3]?.args).toEqual(['exec', 'tsx', 'scripts/compile-referenced-projects.ts', 'client']) }) it('annotates a failure as a warning and passes in report mode', () => { @@ -52,6 +56,7 @@ describe('pre-merge check', () => { expect(checkMode('off')).toBe('off') expect(checkMode('nonsense')).toBe('report') expect(checkMode(undefined)).toBe('report') + expect(checkMode('')).toBe('report') expect(failureDecision('report', 'the install step failed')).toEqual({ line: '::warning title=Pre-Merge Check::the merged tree fails its pre-merge check: the install step failed', exitCode: 0, diff --git a/scripts/pre-merge-check.ts b/scripts/pre-merge-check.ts index 832cac8735..576b6ee7f9 100644 --- a/scripts/pre-merge-check.ts +++ b/scripts/pre-merge-check.ts @@ -28,7 +28,7 @@ export interface PreMergeStep { */ export function preMergeSteps(root: string): PreMergeStep[] { return [ - { name: 'versions', command: process.execPath, args: [join(root, 'scripts', 'verify-version-sync.ts')] }, + { name: 'versions', command: process.execPath, args: [join(root, 'scripts', 'verify-version-sync.ts'), root] }, { name: 'install', command: 'pnpm', args: ['install', '--frozen-lockfile'] }, { name: 'host contracts', command: 'pnpm', args: ['exec', 'tsx', 'scripts/compile-referenced-projects.ts', 'host-contracts'] }, { name: 'client', command: 'pnpm', args: ['exec', 'tsx', 'scripts/compile-referenced-projects.ts', 'client'] }, @@ -49,7 +49,7 @@ export interface FailureDecision { * @param value - The `CHECK_MODE` value, when the environment provides one. * @returns The mode; a missing or unknown value reports instead of blocking. */ -export function checkMode(value: string | undefined = process.env.CHECK_MODE): CheckMode { +export function checkMode(value: string | undefined): CheckMode { return value === 'off' || value === 'block' ? value : 'report' } @@ -124,7 +124,7 @@ if (isMain) { if (outcome.ok) { process.stdout.write('pre-merge-check: passed\n') } else { - const decision = failureDecision(checkMode(), outcome.reason ?? 'unknown reason') + const decision = failureDecision(checkMode(process.env.CHECK_MODE), outcome.reason ?? 'unknown reason') ;(decision.exitCode === 0 ? process.stdout : process.stderr).write(`${decision.line}\n`) process.exitCode = decision.exitCode } diff --git a/scripts/verify-version-sync.spec.ts b/scripts/verify-version-sync.spec.ts index e928e2ab7a..bd6ea13360 100644 --- a/scripts/verify-version-sync.spec.ts +++ b/scripts/verify-version-sync.spec.ts @@ -1,8 +1,12 @@ +import { spawnSync } from 'node:child_process' import { mkdirSync, mkdtempSync, rmSync, writeFileSync } from 'node:fs' import { tmpdir } from 'node:os' import { dirname, join } from 'node:path' import { describe, expect, it } from 'vitest' -import { describeVersions, mismatchedVersions, readVersions, releasedManifests } from './verify-version-sync.ts' +import { describeVersions, mismatchedVersions, readVersions, releasedManifests, runVersionCheck, versionsSynchronized } from './verify-version-sync.ts' + +/** Path of the command the pre-merge check spawns. */ +const script = join(import.meta.dirname, 'verify-version-sync.ts') /** * Build a repository holding the given manifests. @@ -20,12 +24,14 @@ function repository(files: Record): string { } describe('released manifests', () => { - it('collects the packages and applications of the root version and skips other version lines', () => { + it('collects the package groups and the applications, and nothing deeper or beside them', () => { const root = repository({ 'package.json': '1.0.0', 'packages/core/session/package.json': '1.0.0', - 'packages/AGENTS.md': null, + 'packages/core/package.json': null, + 'packages/README.md': null, 'apps/cli/package.json': '1.0.0', + 'apps/cli/nested/package.json': '1.0.0', 'vendor/cordis/package.json': '4.0.5', 'native/system/package.json': '0.1.2', }) @@ -35,10 +41,41 @@ describe('released manifests', () => { rmSync(root, { recursive: true, force: true }) } }) + + it('fails instead of reporting a narrowed corpus', () => { + const root = repository({ 'package.json': '1.0.0', 'packages/README.md': null, 'apps/README.md': null }) + try { + expect(() => releasedManifests(root)).toThrow(/no released manifest/) + } finally { + rmSync(root, { recursive: true, force: true }) + } + }) + + it('fails when a directory cannot be read', () => { + const root = repository({ 'package.json': '1.0.0', 'packages/core/session/package.json': '1.0.0' }) + try { + rmSync(join(root, 'apps'), { recursive: true, force: true }) + expect(() => releasedManifests(root)).toThrow(/cannot read apps\//) + } finally { + rmSync(root, { recursive: true, force: true }) + } + }) }) describe('version synchronization', () => { - it('names a manifest left on another version or without one', () => { + it('accepts a tree whose released manifests all carry the root version', () => { + const root = repository({ 'package.json': '3.0.0', 'packages/core/session/package.json': '3.0.0', 'apps/web/package.json': '3.0.0' }) + try { + const report = readVersions(root) + expect(mismatchedVersions(report.rootVersion ?? '', report.manifests)).toEqual([]) + expect(versionsSynchronized(report)).toBe(true) + expect(describeVersions(report)).toEqual(['2 released manifest(s) carry 3.0.0']) + } finally { + rmSync(root, { recursive: true, force: true }) + } + }) + + it('rejects a manifest on another version or without one', () => { const root = repository({ 'package.json': '2.0.0', 'packages/core/session/package.json': '2.0.0', @@ -46,7 +83,9 @@ describe('version synchronization', () => { 'apps/cli/package.json': null, }) try { - expect(describeVersions(readVersions(root))).toEqual([ + const report = readVersions(root) + expect(versionsSynchronized(report)).toBe(false) + expect(describeVersions(report)).toEqual([ '2 of 3 released manifest(s) do not carry the root version 2.0.0:', ' apps/cli/package.json: no version', ' packages/core/agent/package.json: 1.0.0', @@ -56,17 +95,37 @@ describe('version synchronization', () => { } }) - it('accepts a tree whose released manifests all carry the root version', () => { - const root = repository({ - 'package.json': '3.0.0', - 'packages/core/session/package.json': '3.0.0', - 'apps/web/package.json': '3.0.0', - 'vendor/cordis/package.json': '4.0.5', - }) + it('rejects an empty corpus and a missing root version', () => { + expect(versionsSynchronized({ rootVersion: '1.0.0', manifests: [] })).toBe(false) + expect(versionsSynchronized({ rootVersion: undefined, manifests: [{ path: 'apps/cli/package.json', version: '1.0.0' }] })).toBe(false) + expect(versionsSynchronized({ rootVersion: undefined, manifests: [] })).toBe(false) + }) +}) + +describe('verify-version-sync command', () => { + it('exits 0 on a synchronized tree and 1 with the mismatch named', () => { + const good = repository({ 'package.json': '1.0.0', 'packages/core/session/package.json': '1.0.0', 'apps/web/package.json': '1.0.0' }) + const bad = repository({ 'package.json': '2.0.0', 'packages/core/session/package.json': '1.0.0', 'apps/web/package.json': '2.0.0' }) try { - const report = readVersions(root) - expect(mismatchedVersions(report.rootVersion ?? '', report.manifests)).toEqual([]) - expect(describeVersions(report)).toEqual(['2 released manifest(s) carry 3.0.0']) + expect(runVersionCheck(good).ok).toBe(true) + const passed = spawnSync(process.execPath, [script, good], { encoding: 'utf8' }) + expect(passed.status).toBe(0) + expect(passed.stdout).toContain('2 released manifest(s) carry 1.0.0') + const failed = spawnSync(process.execPath, [script, bad], { encoding: 'utf8' }) + expect(failed.status).toBe(1) + expect(failed.stderr).toContain('packages/core/session/package.json: 1.0.0') + } finally { + rmSync(good, { recursive: true, force: true }) + rmSync(bad, { recursive: true, force: true }) + } + }) + + it('exits 1 when the tree cannot be read', () => { + const root = mkdtempSync(join(tmpdir(), 'dsh-version-sync-empty-')) + try { + const result = spawnSync(process.execPath, [script, root], { encoding: 'utf8' }) + expect(result.status).toBe(1) + expect(result.stderr).toContain('verify-version-sync:') } finally { rmSync(root, { recursive: true, force: true }) } diff --git a/scripts/verify-version-sync.ts b/scripts/verify-version-sync.ts index a194665e64..9c2d14b400 100644 --- a/scripts/verify-version-sync.ts +++ b/scripts/verify-version-sync.ts @@ -1,10 +1,11 @@ /** - * Check that every released manifest carries the root version. A release - * synchronization bumps them together, so a mismatch means a merge combined two - * version lines: a pull request that bumps versions merged onto a master that - * added a package, for instance. + * Check that every released manifest carries the root version. A release synchronization bumps + * them together, so a mismatch means a merge combined two version lines: a pull request that bumps + * versions merged onto a master that added a package, for instance. * - * The check reads manifests only, so it needs no installed dependency. + * The released manifests are the ones the release tooling rewrites, `packages//` + * and `apps/`. Reading them needs no installed dependency, so the check runs before an + * install. */ import { readFileSync, readdirSync, statSync } from 'node:fs' @@ -19,14 +20,31 @@ export interface ManifestVersion { readonly version: string | undefined } +/** Whether a tree is synchronized, with the lines describing it. */ +export interface VersionOutcome { + readonly ok: boolean + readonly lines: string[] +} + /** The root version and every released manifest's version. */ export interface VersionReport { readonly rootVersion: string | undefined readonly manifests: ManifestVersion[] } -/** Directories that hold released manifests below the repository root. */ -const MANIFEST_DIRECTORIES = ['packages', 'apps'] as const +/** Where released manifests live: the package groups and the applications. */ +const PACKAGE_ROOT = 'packages' +const APPLICATION_ROOT = 'apps' + +/** + * Report whether an error only means the path is not there. + * @param error - Caught value. + * @returns Whether the entry is absent, as a missing file or a parent that is one. + */ +function isMissing(error: unknown): boolean { + const code = (error as NodeJS.ErrnoException).code + return code === 'ENOENT' || code === 'ENOTDIR' +} /** * Describe an error for a reader. @@ -37,60 +55,85 @@ function messageOf(error: unknown): string { return error instanceof Error ? error.message : String(error) } +/** + * Test whether a path is a regular file, treating only "no such file" as a no. + * @param path - Absolute path. + * @returns Whether the file exists. + */ +function isFile(path: string): boolean { + try { + return statSync(path).isFile() + } catch (error) { + if (isMissing(error)) return false + throw new Error(`cannot read ${path}: ${messageOf(error)}`) + } +} + +/** + * Test whether a path is a directory. + * @param path - Absolute path. + * @returns Whether the entry is a directory. + */ +function isDirectory(path: string): boolean { + try { + return statSync(path).isDirectory() + } catch (error) { + if (isMissing(error)) return false + throw new Error(`cannot read ${path}: ${messageOf(error)}`) + } +} + +/** + * List the entries of a directory, naming the directory when it cannot be read. + * @param path - Absolute directory path. + * @param label - Repository-relative label for the message. + * @returns Entry names. + */ +function entries(path: string, label: string): string[] { + try { + return readdirSync(path) + } catch (error) { + throw new Error(`cannot read ${label}: ${messageOf(error)}`) + } +} + /** * Read the version one `package.json` declares. * @param path - Absolute path of the manifest. * @returns The declared version, or `undefined` when it declares none. */ function versionOf(path: string): string | undefined { - const manifest: unknown = JSON.parse(readFileSync(path, 'utf8')) - if (typeof manifest !== 'object' || manifest === null) return undefined - const version: unknown = (manifest as { version?: unknown }).version - return typeof version === 'string' ? version : undefined + try { + const manifest: unknown = JSON.parse(readFileSync(path, 'utf8')) + if (typeof manifest !== 'object' || manifest === null) return undefined + const version: unknown = (manifest as { version?: unknown }).version + return typeof version === 'string' ? version : undefined + } catch (error) { + throw new Error(`cannot read ${path}: ${messageOf(error)}`) + } } /** - * Collect the manifests that must carry the root version: `packages//` and - * `apps/`. A directory that cannot be read, and a walk that finds nothing, are - * errors rather than a clean tree, so a narrowed corpus cannot pass the check. + * Collect the manifests the release tooling rewrites: `packages///package.json` + * and `apps//package.json`. A directory that cannot be read, and a walk that finds + * nothing, are errors rather than a clean tree, so a narrowed corpus cannot pass the check. * @param root - Repository root. * @returns Manifest paths relative to the root, sorted. */ export function releasedManifests(root: string): string[] { const paths: string[] = [] - for (const directory of MANIFEST_DIRECTORIES) { - let groups: string[] - try { - groups = readdirSync(join(root, directory)) - } catch (error) { - throw new Error(`cannot read ${directory}/: ${messageOf(error)}`) - } - for (const group of groups) { - const groupDirectory = join(root, directory, group) - let isDirectory: boolean - try { - isDirectory = statSync(groupDirectory).isDirectory() - } catch (error) { - throw new Error(`cannot read ${directory}/${group}: ${messageOf(error)}`) - } - if (!isDirectory) continue - const manifest = join(groupDirectory, 'package.json') - try { - if (statSync(manifest).isFile()) paths.push(`${directory}/${group}/package.json`) - } catch { - // A directory without a manifest is not a package. - } - for (const entry of readdirSync(groupDirectory)) { - const nested = `${directory}/${group}/${entry}/package.json` - try { - if (statSync(join(root, nested)).isFile()) paths.push(nested) - } catch { - // Not a package directory. - } - } + for (const group of entries(join(root, PACKAGE_ROOT), `${PACKAGE_ROOT}/`)) { + if (!isDirectory(join(root, PACKAGE_ROOT, group))) continue + for (const entry of entries(join(root, PACKAGE_ROOT, group), `${PACKAGE_ROOT}/${group}/`)) { + const path = `${PACKAGE_ROOT}/${group}/${entry}/package.json` + if (isFile(join(root, path))) paths.push(path) } } - if (paths.length === 0) throw new Error(`no released manifest was found under ${MANIFEST_DIRECTORIES.map(directory => `${directory}/`).join(' or ')}`) + for (const application of entries(join(root, APPLICATION_ROOT), `${APPLICATION_ROOT}/`)) { + const path = `${APPLICATION_ROOT}/${application}/package.json` + if (isFile(join(root, path))) paths.push(path) + } + if (paths.length === 0) throw new Error(`no released manifest was found under ${PACKAGE_ROOT}/ or ${APPLICATION_ROOT}/`) return paths.sort() } @@ -116,9 +159,10 @@ export function readVersions(root: string): VersionReport { } /** - * Report whether every released manifest carries the root version. + * Report whether every released manifest carries the root version. A tree without a root version, + * or with no released manifest at all, is not synchronized. * @param report - Root version and manifests. - * @returns Whether the tree is synchronized; a tree without a root version or without manifests is not. + * @returns Whether the tree is synchronized. */ export function versionsSynchronized(report: VersionReport): boolean { const { rootVersion, manifests } = report @@ -126,7 +170,7 @@ export function versionsSynchronized(report: VersionReport): boolean { } /** - * Describe a version mismatch for a reader. + * Describe a version report for a reader. * @param report - Root version and manifests. * @returns One line when the tree is consistent, otherwise a header and one line per mismatched manifest. */ @@ -141,14 +185,24 @@ export function describeVersions(report: VersionReport): string[] { ] } +/** + * Run the check over one repository. + * @param root - Repository root. + * @returns Whether the tree is synchronized, with the lines describing it. + */ +export function runVersionCheck(root: string): VersionOutcome { + const report = readVersions(root) + return { ok: versionsSynchronized(report), lines: describeVersions(report) } +} + const invokedPath = process.argv[1] const isMain = invokedPath !== undefined && import.meta.url === pathToFileURL(resolve(invokedPath)).href if (isMain) { + const root = process.argv[2] ?? resolve(import.meta.dirname, '..') try { - const report = readVersions(resolve(import.meta.dirname, '..')) - const synchronized = versionsSynchronized(report) - for (const line of describeVersions(report)) (synchronized ? console.log : console.error)(`verify-version-sync: ${line}`) - if (!synchronized) process.exitCode = 1 + const { ok, lines } = runVersionCheck(resolve(root)) + for (const line of lines) (ok ? console.log : console.error)(`verify-version-sync: ${line}`) + if (!ok) process.exitCode = 1 } catch (error) { console.error(`verify-version-sync: ${messageOf(error)}`) process.exitCode = 1 From 5d0b96b5bd072e1a6295675b3dc529415efc8c3b Mon Sep 17 00:00:00 2001 From: imccyu Date: Mon, 26 Oct 2026 23:52:18 +0800 Subject: [PATCH 6/6] docs: state the pre-merge order the check runs, without a stray blank line --- .github/AGENTS.md | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/.github/AGENTS.md b/.github/AGENTS.md index c3e2ee32ee..bde3142e27 100644 --- a/.github/AGENTS.md +++ b/.github/AGENTS.md @@ -2,9 +2,8 @@ Run jobs on Windows runners (`windows-*` labels) under native `pwsh`. Native Windows pull-request jobs live in `ci-windows.yml` and Python pull-request jobs in `ci-python.yml`, so each PR workflow can be re-run independently; their checks feed the required `windows checks passed` and `python checks passed` verdicts, as Linux jobs in `ci.yml` feed `all checks passed`; Wine runs Windows Node on hosted Linux only in `ci-master.yml`. Python runtime CI checks Linux/Windows x64 on pull requests and Linux ARM64 plus both macOS architectures on master pushes; releases retain all five targets ([platform policy](workflows/ci-master.yml)). `ci.yml`, `ci-windows.yml`, and `ci-python.yml` are pull-request-only. Master-only platform checks, Linux/Windows self-hosted standbys, and manual runner benchmarks live in `ci-master.yml`, which listens to master pushes and `workflow_dispatch`, not `pull_request`; separating workflow triggers keeps master-only jobs out of PR check panels. The master standbys validate the self-hosted failover targets; preserve the existing per-platform switches (values `selfhosted` for the in-house standbys and `blacksmith` for Blacksmith's hosted runners) and the Dependabot hosted fallback under the `selfhosted` values ([failover runbook](../.agents/notes/implemented/process/2026-07-26-ci-failover-runbook.md), [blacksmith failover leg note](../.agents/notes/implemented/process/2026-09-09-blacksmith-failover-leg.md)). - The manual [Windows packaging workflow](workflows/windows-package.yml) uses the dedicated self-hosted Windows x64 label `dsh-win-package-trial` under native `pwsh`; preserve this routing separately from the automatic CI lanes. -The merge checks run from three workflows named `Policy`, which run on `pull_request_target` so their definitions come from master, and call the `deepseek-harness-check` action directly: `pr-check-on-head-branch.yml` on pull-request updates gives `Policy / Master Change` and `Policy / Pre-Merge Check`, `pr-check-after-approve.yml` on pull-request updates and submitted or dismissed reviews gives `Policy / Approve Check` and `Policy / Reviewer Evaluate`, and `pr-check-on-base-branch.yml` on master pushes re-runs the head-branch checks as `Policy / Recheck`. Branch rules require those check names, so keep the job names and the workflow name. Each check reads its mode from a repository variable: `MERGE_POLICY_MASTER_DIFF`, `MERGE_POLICY_PRE_MERGE`, `MERGE_POLICY_HEAD_DIFF`, and `MERGE_POLICY_REVIEW`, each `off` (skip), `report` (run without failing the job), or `block` (fail the job); a missing value or an unknown one counts as `report`. `Policy / Pre-Merge Check` merges the commit `Policy / Master Change` reports, so it runs only after that job and stays off while `MERGE_POLICY_MASTER_DIFF` is `off`; a merge it cannot do and a build that fails both follow its own mode, so `report` warns and skips the build while `block` fails the job. `Policy / Pre-Merge Check` runs `scripts/pre-merge-check.ts` under plain Node and knows nothing else about the merged tree: that script installs, checks that every released manifest carries the root version, and builds the Host and Client contracts, so change it in one pull request together with the build it orchestrates. +The merge checks run from three workflows named `Policy`, which run on `pull_request_target` so their definitions come from master, and call the `deepseek-harness-check` action directly: `pr-check-on-head-branch.yml` on pull-request updates gives `Policy / Master Change` and `Policy / Pre-Merge Check`, `pr-check-after-approve.yml` on pull-request updates and submitted or dismissed reviews gives `Policy / Approve Check` and `Policy / Reviewer Evaluate`, and `pr-check-on-base-branch.yml` on master pushes re-runs the head-branch checks as `Policy / Recheck`. Branch rules require those check names, so keep the job names and the workflow name. Each check reads its mode from a repository variable: `MERGE_POLICY_MASTER_DIFF`, `MERGE_POLICY_PRE_MERGE`, `MERGE_POLICY_HEAD_DIFF`, and `MERGE_POLICY_REVIEW`, each `off` (skip), `report` (run without failing the job), or `block` (fail the job); a missing value or an unknown one counts as `report`. `Policy / Pre-Merge Check` merges the commit `Policy / Master Change` reports, so it runs only after that job and stays off while `MERGE_POLICY_MASTER_DIFF` is `off`; a merge it cannot do and a build that fails both follow its own mode, so `report` warns and skips the build while `block` fails the job. `Policy / Pre-Merge Check` runs `scripts/pre-merge-check.ts` under plain Node and knows nothing else about the merged tree: that script checks the released manifests against the root version, installs, and builds the Host and Client contracts, so change it in one pull request together with the build it orchestrates. The manual Desktop publication workflow selects a successful package run and attempt, then routes each retained build through `DESKTOP_PUBLISH_RUNNERS` to its original machine's unique label. Keep Windows run steps on native PowerShell and Mac run steps on the default POSIX shell; shared entrypoints are Node scripts. Test and production jobs use separate `desktop-test` and `desktop-production` approval Environments after a credential-free preparation job.