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.
This commit is contained in:
1 parent
c3998dde7c
commit
a518d10c95
6 files changed
+140
-44
No files matched your search
+1
-2
@@ -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.
|
||||
@@ -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
|
||||
@@ -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<string, unknown> {
|
||||
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')
|
||||
})
|
||||
})
|
||||
@@ -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'])
|
||||
})
|
||||
|
||||
+41
-20
@@ -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<number>((settle) => {
|
||||
// Listen before anything else can throw: a pnpm that cannot start must not leave the check hanging.
|
||||
const installed = await new Promise<InstallOutcome>((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 }
|
||||
}
|
||||
|
||||
|
||||
@@ -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/<group>/<package>` and `apps/<application>`.
|
||||
* Collect the manifests that must carry the root version: `packages/<group>/<package>` and
|
||||
* `apps/<application>`. 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
|
||||
}
|
||||
}
|
||||
Reference in new issue
Block a user