MemberJunction / MemberJunction/MJ

Repo-wide compliance gates never run on PRs that don't touch their host package

Open
#4,326 0 comments 0 reactions 0 assignees View on GitHub
chore priority: medium
Dominant language
TSQL
Stars
29
Forks
6
Avg merge
2d 1h
Merged PRs (30d)
323

Description

## Summary

The three repo-wide compliance gates all live inside a package's vitest suite and scan `packages/**` from there. PR unit-test runs are scoped to changed packages plus their dependents, and the packages hosting these gates are dependencies of nearly everything rather than dependents of anything. So on any PR that does not touch the host package, the gate does not run at all. A violation introduced anywhere else passes PR CI and is caught only by the post-merge backstop on `next`, where it breaks the default branch instead of the PR that caused it.

The gates:

| Gate | Host package | Scans |
|---|---|---|
| `PrimaryKeyCompliance.test.ts` | `@memberjunction/core` | `packages/**` |
| `UUIDCompliance.test.ts` | `@memberjunction/global` | `packages/**` |
| `MultiProviderCompliance.test.ts` | `@memberjunction/global` | `packages/**` |

## Why it happens

`.github/workflows/test.yml` sets the PR scope in the "Determine test scope" step:

```
FILTER="--filter=...[origin/$PR_BASE_REF]"
```

Turbo's `...[ref]` selects packages changed since the ref **plus everything that depends on them**. `@memberjunction/core` and `@memberjunction/global` sit at the bottom of the dependency graph, so a change in a leaf package never selects them.

The workflow comment at that step notes that a PR touching core or global selects essentially the whole repo, 309 of 313 packages measured. That is the reverse direction and is true. It is the forward direction that is missing.

## Verified

On PR #4319, which changed only `packages/Angular/Generic/whiteboard`:

```
$ grep -c "@memberjunction/core:test" # 0
$ grep -c "@memberjunction/global:test" # 0
```

Neither host package's test task appeared in any of the six shards, so none of the three gates ran. `ci-standards.yml` does not cover them either; it runs only `@memberjunction/standards`.

## Impact

Low likelihood per PR, high annoyance when it lands. The failure mode is a red `next` after merge rather than a red PR, and the person who has to diagnose it is whoever merges next, not the author who introduced the violation. `PrimaryKeyCompliance` is the most exposed of the three because it deliberately ships with no baseline file, so every violation is a hard failure.

## Suggested fix

The `guards` job in the same workflow already runs on every PR with no turbo filter and no `pnpm install`, and it hosts several dependency-free gates invoked in exactly the shape these need:

```yaml
- run: node scripts/check-dom-spec-placement.mjs packages
- run: node scripts/check-spec-antipatterns.mjs packages
```

Porting the three scans to `.mjs` guards in that job would make them run on every PR and would also move them off the critical path of the sharded suite, since they are pure text scans that need no build. The vitest versions could then be deleted or kept as thin wrappers.

The cheaper alternative is to force the two host packages into the PR filter, but the workflow passes `TURBO_FILTER` as a single quoted argument in the build step, so adding a second `--filter=` flag needs care there.

## Secondary, same files

None of the three gates asserts that the scan actually found files. `findTsFiles` returns `[]` through its `fs.existsSync` early return and every gate then passes. The realistic trigger is a move rather than a deletion: relocating a gate one directory deeper turns `SCAN_ROOT` into the host package's own directory and silently narrows the scan to one package while staying green. One assertion per gate on a minimum scanned-file count would close it.

## Context

Found while reviewing #4304, which adds `PrimaryKeyCompliance`. Not a defect in that PR; it inherits an existing placement pattern. Filed separately so the PR is not held up.

Contributor guide

Open the contributing guide

Research direction

Start with the guards job and the Determine test scope step in .github/workflows/test.yml, then inspect the three compliance tests and the existing scripts/check-dom-spec-placement.mjs and scripts/check-spec-antipatterns.mjs patterns. Verify the chosen approach on a PR that changes only a leaf package, and confirm all three compliance gates run there while still scanning packages/**; review ci-standards.yml for unrelated coverage.

Written by the indexing model from the issue text.

Assessment

Tech stack
github-actions, node.js, typescript
Domain
build-system, ci-cd, testing-qa
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Active
Clarity
Mostly clear
Newbie friendliness
70/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.