Skip to content

Add PR gate workflow (build, type-check, unit)#1038

Merged
pavlovic-ivan merged 1 commit into
G-Research:masterfrom
dev-milos:ci/add-pr-gate
Jul 23, 2026
Merged

Add PR gate workflow (build, type-check, unit)#1038
pavlovic-ivan merged 1 commit into
G-Research:masterfrom
dev-milos:ci/add-pr-gate

Conversation

@dev-milos

@dev-milos dev-milos commented Jul 22, 2026

Copy link
Copy Markdown
Contributor

Adds the first CI gate: npm cibuildchecktest:unit on every PR to master. Nothing currently builds or runs this code before merge.

Validated on Linux (Node 22.19.0)

Install / Build / Type-check   pass
Unit gate    1 failed | 940 passed | 4 skipped (945)
Total        3m29s

Excluded

  • test:browser — 3600s wall budget, and its Playwright config reserves workers from scripts/testing-v2/ledger.mjs, which exists to share one dev box between concurrent sessions and is inert on a runner.
  • test:e2e — needs Docker and real worktrees.

The one failure

tests2/core/file-mentions-resolve.test.ts:698 calls execFileSync("mkfifo", ...), which this repo's own tier-1 spawn guard blocks. Introduced by #1030 and merged red because nothing ran it. Fixed separately so this PR stays reviewable on its own.

A clean macOS checkout fails 14 files, but 13 are local-environment artifacts (TMPDIR symlink resolution, and Node below the engines floor of 22.19 where native .ts loading landed). All 13 pass on Linux.

Rollout

Keep non-required until it has been observed on a few PRs.

@dev-milos
dev-milos requested a review from SuuBro as a code owner July 22, 2026 09:20
@greptile-apps

greptile-apps Bot commented Jul 22, 2026

Copy link
Copy Markdown

Greptile Summary

This PR adds a GitHub Actions gate for pull requests and pushes to master. The main changes are:

  • Installs dependencies with Node 22.19.0.
  • Builds and type-checks the project.
  • Runs the unit test suite.

Confidence Score: 5/5

This looks safe to merge.

  • The previously failing FIFO test no longer invokes the blocked mkfifo command.
  • No blocking issue remains in the changed workflow.

Important Files Changed

Filename Overview
.github/workflows/build-unit-gate.yml Adds the build, type-check, and unit-test workflow with pinned checkout and Node setup actions.

Reviews (4): Last reviewed commit: "Add PR gate workflow (build, type-check,..." | Re-trigger Greptile

Comment thread .github/workflows/pr.yml Outdated
run: npm run check

- name: Unit gate
# vitest.config.ts pins FIXED_UNIT_WORKERS = 3; VITEST_MAX_WORKERS can

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Unit Gate Always Fails

test:unit runs the POSIX FIFO test in tests2/core/file-mentions-resolve.test.ts, which calls execFileSync("mkfifo", ...) after the tier-1 child-process guard is installed. On ubuntu-latest, the guard throws on every attempt, so this new workflow always finishes red instead of providing a usable PR signal.

Context Used: AGENTS.md (source)

@dev-milos
dev-milos force-pushed the ci/add-pr-gate branch 3 times, most recently from ec87bca to 19c1b6a Compare July 22, 2026 09:30
@dev-milos
dev-milos marked this pull request as draft July 22, 2026 09:36
@dev-milos
dev-milos force-pushed the ci/add-pr-gate branch 3 times, most recently from c0cdad7 to 88c4c69 Compare July 23, 2026 07:29
First CI gate for the repo. Runs on every PR to master:
npm ci -> build -> check -> test:unit.

Scope is the fast tier only. Deliberately excluded:
  - test:browser: budgeted at 3600 s wall, and playwright-v2.config.ts
    reserves workers from scripts/testing-v2/ledger.mjs, which exists to
    share one 24-core dev box between concurrent agent sessions. On a
    single-tenant runner that machinery is inert and misleading.
  - test:e2e: needs Docker and real worktrees.
Both need to be made runner-shaped before they belong in a PR gate.

Verified from a clean detached worktree at origin/master (macOS, 24 core):
npm ci 14s, build 12s, check 30s, test:unit 78s.

That run also surfaced three pre-existing unit-gate failures (14 files /
56 tests), which is the reason this gate is worth having:

  1. macOS TMPDIR symlink (6 files). os.tmpdir() is /var/folders/... but
     its realpath is /private/var/folders/... Tests such as
     tests2/integration/project-ui-api.test.ts:55 compare a server-
     canonicalized path against the raw one by strict equality. Linux has
     no such indirection, so these pass in CI and fail only on macOS.

  2. Node below engines (7 files). engines.node is >=22.19.0; native .ts
     loading arrived in 22.18. On 22.17 the extension-host suites die with
     'Unknown file extension ".ts"'. npm ci already warns EBADENGINE --
     the warning is load-bearing, not cosmetic. This workflow pins 22.19.0.

  3. A genuinely broken test (1 file). tests2/core/file-mentions-
     resolve.test.ts:698 calls execFileSync("mkfifo"), which this repo's
     own tier-1 spawn guard blocks. Introduced by G-Research#1030 and merged red
     because nothing ran it.

(1) and (2) are laptop-specific and disappear on a Linux runner. (3) is
real and will make this gate red on arrival. That is intended: it is
fixed separately so this PR stays reviewable on its own, and the gate
should stay non-required until it has been observed on a few PRs.
@dev-milos
dev-milos marked this pull request as ready for review July 23, 2026 11:39
@pavlovic-ivan
pavlovic-ivan merged commit fb4e444 into G-Research:master Jul 23, 2026
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants