fix(typecheck): catch undefined identifiers in server tests

[kkroo]

Thinking Path

BLO-24983 wedged master for ~3.7 hours: human-gated-ageing.test.ts called buildHumanGatedAgeingReport, a symbol that exists nowhere, so it threw ReferenceError on every run and every merge-queue batch failed General tests (server 1/4). The instance was fixed by #1275. This addresses why it was possible.

server/tsconfig.json excludes src/__tests__ — but it is both the build config and the typecheck config (server build: tsc && … and server typecheck: … tsc --noEmit share it). The exclude is correct for the build, since tests should not land in dist/. Its side effect was that tests were never typechecked at all.

I measured before choosing a fix, because "just include the tests" was the obvious move and I did not trust it. Including src/__tests__ surfaces 740 errors — mostly fixture typing (TS2345 ×196, TS2322 ×85, TS2339 ×43) plus 91 TS5097 config noise. That is a real project, not a config tweak, and gating on it would mean a permanently red check.

But the bug class that caused the outage is much narrower than "tests are fully typed". Filtering to undefined-identifier codes gives exactly 1 pre-existing error. So the whole outage was preventable by a gate that is one fix away from green today. That is the version I built.

What Changed

• server/tsconfig.typecheck.json — typecheck-only view that includes tests, noEmit. Build and dist/ are untouched.
• scripts/check-test-undefined-symbols.mjs — runs tsc against it and fails only on TS2304/TS2552/TS2662/TS2663.
• package.json — check:test-undefined-symbols, so it runs locally exactly as in CI.
• .github/workflows/pr.yml — one step in the existing Typecheck + Release Registry job. No new job, no trigger change.
• worktree-config.test.ts — fixes the single pre-existing offender: it used the type PaperclipConfig without importing it. Same bug class as the outage, but a type, so it erased at runtime and the test passed anyway. A nice demonstration that this catches the latent form too.

Two deliberate properties, both learned from guards that lied here before:

1. It keys on the filtered diagnostics, never on tsc's exit code — that project exits nonzero by design. The script and the config both say so in comments, because "fix the script to check the exit code" is the obvious wrong change for a future reader.
2. A gate that cannot run must not report clean. If tsc produces zero diagnostics and exits nonzero, it fails with a distinct "could not run" message and dumps the output.

Risks

Low, and bounded to CI. No production code, no build change.

The honest limitation: this does not make tests type-safe. It catches names that do not resolve, which is the class that fails at runtime. The other 739 errors stay unchecked, and the script says so out loud on success (739 other type errors present and intentionally not gated) rather than implying coverage it does not have. Closing that gap is worth its own ticket; I did not want to hide a 740-error backlog behind a green check.

Second risk: the new step adds a tsc pass to an already 40-minute job. It reuses the same install and took well under a minute locally.

Verification

Ran in a clean worktree off origin/master (1eab22741), install --offline:

control	expected	result
gate on current master	pass	ok no undefined identifiers … (739 other type errors …) rc=0
re-introduce the BLO-24983 bug	fail, naming it	FAIL … human-gated-ageing.test.ts(611,20): error TS2304: Cannot find name 'buildHumanGatedAgeingReport'
remove the import this PR adds	fail, naming it	FAIL … worktree-config.test.ts(40,90): error TS2304: Cannot find name 'PaperclipConfig'
delete the tsconfig (gate cannot run)	fail loudly, not pass	FAIL cannot run: …/tsconfig.typecheck.json not found rc=1

Also: baseline pnpm run typecheck still rc=0 (build config untouched); worktree-config.test.ts 14/14; pr.yml parses under ruby -ryaml; invoked through pnpm run exactly as CI does.

All four PR policy gates pass locally. I checked the prefix against the real gate rather than assuming: ci( trips MISMATCH detection because the diff adds a .mjs, so this is titled fix(.

• I searched open PRs for similar or duplicate prior work before writing this — no open PR touches server/tsconfig.json or tsconfig.typecheck.json; I opened the six tsconfig-keyword candidates (fix(github-webhook): drop claude[bot] Code Review paused-notice reviews (BLO-23059) #1255, Reject page/perPage on GET /companies/:companyId/issues (BLO-24495) #1285, feat(sweep): add human-gated ageing escalation (BLO-19130) #1241, fix: require payload.title on server-internal approval producers (BLO-22705) #1130, fix(heartbeat): stop self-authored status comments from defeating the rewake throttle (BLO-23081) #1165, feat(vendor): bring claude_k8s adapter in-tree and retire CLAUDE_K8S_REF (BLO-17980) #1092) and only feat(vendor): bring claude_k8s adapter in-tree and retire CLAUDE_K8S_REF (BLO-17980) #1092 touches a tsconfig, a vendored adapter's.

Refs BLO-24983

Model Used

Claude Opus 5 (1M context)

[allyblockcast]

🔗 Paperclip issue: BLO-24983

[allyblockcast]

🔗 Paperclip issue: BLO-24983

[cursor]

Bugbot is not enabled for your account, so this pull request was not reviewed.

Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs.

[allyblockcast]

Ally — Consolidated PR Review

Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: d8e1481

Looks good. The typecheck-only config preserves the production build exclusion while making test sources visible to the focused undefined-name check. The gate correctly treats a failed compiler invocation without diagnostics as a failure rather than a clean result.

Critical Issues (0)

Important Issues (0)

Suggestions (0)

Strengths

• The CI step reuses the existing typecheck job and dependency installation path instead of adding a separate workflow surface.
• The implementation explicitly avoids treating the known test-fixture type-error backlog as a passing full typecheck.

Recommended Action

1. Merge when the normal CI checks are green.

[allyblockcast]

Ally — Consolidated PR Review

Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: d8e1481

Looks good. The typecheck-only config preserves the production build exclusion while making test sources visible to the focused undefined-name check. The gate correctly treats a failed compiler invocation without diagnostics as a failure rather than a clean result.

Critical Issues (0)

Important Issues (0)

Suggestions (0)

Strengths

• The CI step reuses the existing typecheck job and dependency installation path instead of adding a separate workflow surface.
• The implementation explicitly avoids treating the known test-fixture type-error backlog as a passing full typecheck.

Recommended Action

1. Merge when the normal CI checks are green.

Superseded at d8e1481: a concurrent Ally pass found an unresolved Important finding in the fail-closed path.

[allyblockcast]

Ally — Consolidated PR Review

Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: d8e1481

Critical Issues (0)

Important Issues (1)

• [pr-review-toolkit/gstack-review/native-codex] scripts/check-test-undefined-symbols.mjs:63 — The fail-closed condition only rejects a nonzero tsc exit when no diagnostics were emitted. A broken project configuration such as a missing extended config emits a non-target diagnostic (for example TS5083) and exits nonzero, leaving allDiagnostics.length > 0; this script then reaches the success path even though TypeScript did not necessarily typecheck the test program. That makes the gate silently green in precisely the cannot-run scenario it claims to fail closed.
  • Fail on abnormal process termination and compiler/configuration diagnostics independently of the intentionally tolerated source type errors, then retain the filtered undefined-name check for a successful compiler run.

Suggestions (0)

Strengths

• The separate no-emit project preserves the production build boundary while making test source visible to the targeted check.
• The narrow diagnostic filter is a pragmatic immediate guard for the demonstrated runtime-fatal failure class; it should be described as such, not as full test type safety.

Recommended Action

1. Make the gate fail closed for compiler/configuration failures before relying on it in required CI.
2. Keep the narrow undefined-identifier scope for now; the broader test-type-error backlog can be addressed separately.

[allyblockcast]

Ally — Consolidated PR Review

Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 3ee97a0

Prior Findings Dispositioned (1)

• prior:d8e1481 important 1 — still-present — scripts/check-test-undefined-symbols.mjs:95 — the exact-head script rejects a nonzero compiler exit only when allDiagnostics.length === 0; compiler/configuration diagnostics leave that value nonzero and continue to the success path.

Critical Issues (0)

Important Issues (1)

• [pr-review-toolkit/gstack-review/native-codex] scripts/check-test-undefined-symbols.mjs:95 — prior:d8e1481 important 1 — The fail-closed condition only rejects a nonzero tsc exit when no diagnostics were emitted. A broken project configuration such as a missing extended config emits a non-target diagnostic (for example TS5083), making allDiagnostics.length > 0; the script then reaches the success path even though TypeScript did not typecheck the test program.
  • Fail on compiler/configuration diagnostics and abnormal process termination independently of the intentionally tolerated source type errors, then retain the filtered undefined-name check for a successful compiler run.

Suggestions (0)

Strengths

• The typecheck-only config preserves the production build boundary while making test sources visible to the targeted check.
• The narrow diagnostic scope is a pragmatic guard for the demonstrated runtime-fatal undefined-name class.

Recommended Action

1. Make the gate fail closed for compiler/configuration failures before relying on it in required CI.

[allyblockcast]

Ally — Consolidated PR Review

Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 44c1bd3

Prior Findings Dispositioned (1)

• prior:d8e1481 important 1 — fixed — scripts/check-test-undefined-symbols.mjs:102 — the current head runs tsc --showConfig as a separate preflight and exits on a nonzero result before accepting the intentionally tolerated source diagnostics.

Critical Issues (0)

Important Issues (0)

Suggestions (0)

Strengths

• The focused configuration preflight prevents compiler setup failures from being misclassified as tolerated test-source diagnostics.
• The new failure-mode tests cover malformed configuration, abnormal compiler termination, tolerated non-target errors, and undefined identifiers.

Recommended Action

1. Merge when the normal CI checks are green.

[allyblockcast]

Ally — Consolidated PR Review

Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 44c1bd3

Prior Findings Dispositioned (1)

• prior:d8e1481 important 1 — fixed — scripts/check-test-undefined-symbols.mjs:102 — the current head runs tsc --showConfig as a separate preflight and exits on a nonzero result before accepting the intentionally tolerated source diagnostics.

Critical Issues (0)

Important Issues (0)

Suggestions (0)

Strengths

• The focused configuration preflight prevents compiler setup failures from being misclassified as tolerated test-source diagnostics.
• The new failure-mode tests cover malformed configuration, abnormal compiler termination, tolerated non-target errors, and undefined identifiers.

Recommended Action

1. Merge when the normal CI checks are green.