build: make package typecheck tasks check real programs - #698
Merged
Conversation
Eleven typecheck tasks ran plain tsc --noEmit against solution-style tsconfigs (files:[] + references) — non-build tsc ignores references, so they checked an EMPTY program and stayed green with hard type errors in the tree (probe-verified on components, sdk-core, api, cli). - Flip nine to tsc --build tsconfig.json (storybook's existing pattern): api, api-mock, components, config, design-tokens, sdk-next, shared-component-styles, testing, ui-react. Build mode walks the references and emits the per-file declarations project-reference consumers already expect. - apps/cli and packages/sdk-core stay allowlisted KNOWN-VACUOUS: their real programs do not compile yet (hundreds of latent errors plus the @zitadel/source-condition/nodenext-spec-program mismatch that pulls sibling package sources into foreign module settings) — tracked as a follow-up, marked in their moon.yml. - Guard: scripts/check-typecheck-programs.mjs (workspace:check-typecheck, runInCI) re-derives the audit statically, fails on any new vacuous program, and fails when the allowlist goes stale. - Fallout fixed now that the programs are real: components spec fixtures gained the wire's required action.kind (13 sites); config's *.test.ts files moved out of its lib project (exclude/include split) and its lib include gained the imported JSON dirs; six latent config test errors; design-tokens spec includes the script its spec imports; testing's handshake null-guard order plus mock-call index assertions; components' vitest.config.ts left out of the spec program (its types trip over pnpm's duplicate vitest instances; vitest itself exercises the file). - console combobox: annotate an onKeyDown param that fails typecheck on pristine main in a clean store (pre-existing from #673; likely masked in CI by input-keyed task caching).
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
Contributor
|
Contributor
There was a problem hiding this comment.
Pull request overview
This PR makes Moon typecheck tasks actually typecheck real TypeScript programs by switching solution-style tsconfig.json checks to tsc --build, and adds a CI guard that prevents reintroducing “vacuous” non-build tsc --noEmit runs against solution-style configs.
Changes:
- Add
scripts/check-typecheck-programs.mjs+ aworkspace:check-typecheckMoon task (run in CI) to detect vacuous typecheck programs and enforce a shrinking allowlist. - Flip multiple package
typechecktasks/scripts totsc --build tsconfig.jsonso project references are actually typechecked. - Fix newly-exposed TypeScript issues across tests/spec fixtures and a few runtime helpers uncovered once real programs are being checked.
Reviewed changes
Copilot reviewed 31 out of 32 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| scripts/check-typecheck-programs.mjs | New guard script that audits Moon typecheck commands and fails CI on vacuous non-build tsc against solution-style configs (with allowlist). |
| moon.yml | Adds the check-typecheck workspace task (cached, run in CI) that executes the new guard script. |
| packages/ui-react/moon.yml | Switches typecheck task from tsc --noEmit -p ... to tsc --build ... to ensure references are checked. |
| packages/shared-component-styles/moon.yml | Switches typecheck task to tsc --build ... to ensure references are checked. |
| packages/design-tokens/moon.yml | Switches typecheck task to tsc --build ... to ensure references are checked. |
| packages/api/moon.yml | Switches typecheck task to tsc --build ... to ensure references are checked. |
| packages/api-mock/moon.yml | Switches typecheck task to tsc --build ... to ensure references are checked. |
| packages/components/moon.yml | Switches typecheck task to tsc --build ... to ensure references are checked. |
| packages/sdk-core/moon.yml | Documents that typecheck remains intentionally vacuous (tracked follow-up) and is covered by the allowlist. |
| apps/cli/moon.yml | Documents that typecheck remains intentionally vacuous (tracked follow-up) and is covered by the allowlist. |
| packages/testing/package.json | Updates typecheck script to tsc --build .... |
| packages/config/package.json | Updates typecheck script to tsc --build .... |
| packages/sdk-next/package.json | Updates typecheck script to tsc --build .... |
| packages/design-tokens/tsconfig.spec.json | Expands spec program includes to cover a spec-imported script file. |
| packages/config/tsconfig.spec.json | Includes *.test.ts in spec program so tests are typechecked appropriately. |
| packages/config/tsconfig.lib.json | Keeps lib declarations clean by excluding tests and including JSON directories imported by src/. |
| packages/components/tsconfig.spec.json | Adjusts spec program includes to avoid pulling in vitest config (per PR rationale) while still checking specs/types. |
| packages/testing/tests/unit/playwright-config.test.ts | Makes array element access explicit/non-null after length assertions to satisfy stricter typechecking. |
| packages/testing/tests/unit/lifecycle.test.ts | Makes array element access explicit/non-null after length assertions to satisfy stricter typechecking. |
| packages/testing/src/handshake.ts | Tightens handshake-handle validation now that the file is typechecked more strictly. |
| packages/config/src/validate.test.ts | Fixes test typing issues exposed by real program checks (non-null assertions and predicate adjustment). |
| packages/config/src/use-cases.test.ts | Fixes test typing issues exposed by real program checks (casts and safe property access). |
| packages/config/src/presets.test.ts | Fixes test typing issues exposed by real program checks (casts). |
| packages/components/src/orchestrator/zitadel-login.browser.spec.ts | Updates spec fixtures to include required action.kind fields (aligning with flow definition contracts). |
| packages/components/src/orchestrator/theme.browser.spec.ts | Updates spec fixtures to include required action.kind fields. |
| packages/components/src/orchestrator/mandatory-gates.spec.ts | Updates spec fixtures to include required action.kind fields. |
| packages/components/src/orchestrator/exportparts.browser.spec.ts | Updates spec fixtures to include required action.kind fields. |
| packages/components/src/orchestrator/embedding.browser.spec.ts | Updates spec fixtures to include required action.kind fields. |
| packages/components/src/orchestrator/design-templates.spec.ts | Updates spec fixtures to include required action.kind fields. |
| packages/components/src/orchestrator/customization.browser.spec.ts | Updates spec fixtures to include required action.kind fields. |
| apps/demo-next/next-env.d.ts | Updates route-type import path to match current .next/types location. |
| apps/console/src/components/ui/combobox.tsx | Adds an explicit React.KeyboardEvent annotation to resolve a typecheck failure in the console combobox handler. |
- Drop the unused dirname import (CodeQL). - validateHandle also rejects arrays so the first error names the real problem instead of a missing field (Copilot). - The guard deliberately checks every tsc segment of a chained command; fix the stale comment claiming only the last segment matters and scope the failure message to the vacuous segment (Copilot).
…nto build/real-typecheck-programs
…ts individually Codex review round: - P1: the flipped tsc --build typechecks emitted into dist, racing the cleaning tsup/tsdown builds (and in sdk-next colliding head-on with tsup's bundled dist/index.d.ts, same filename). All six dist-emitting lib configs now emit to out-tsc/lib — the layout design-tokens, shared-component-styles, and ui-react already used — so build and typecheck no longer share a single file and need no ordering edge. Project-reference consumers resolve outputs via the updated configs; components' tsdown clean:false comment updated (it cited the old dist-sharing arrangement). - P2: the guard short-circuited whole commands containing --build, so a plain solution-style segment could hide behind a build segment. The classifier is now per-segment, exported, and covered by a node:test regression suite (including the exact mixed-command repro) that the workspace:check-typecheck task runs before the audit. - sdk-next's eslint ignores out-tsc (flat config doesn't inherit .gitignore; the oxlint packages do).
Member
Author
|
Codex review round addressed in 4a415da: P1 (typecheck/build racing over P2 (guard skips mixed commands) — confirmed with your exact repro. The classifier is now per- Full 🤖 Addressed by Claude Code |
fforootd
added a commit
that referenced
this pull request
Aug 4, 2026
…owed server errors (#724) ## Summary Triage of the GitHub **Code quality** dashboards (`/security/quality` and `/security/quality/ai-findings`). Eight findings were reviewed; this PR lands the four that survived verification and have real behaviour impact. **`@zitadel/sdk-nuxt` — anonymous sessions reported as signed in.** The opaque-token fallback authenticated any live session, substituting the literal string `"unknown"` when `/sessions/me` returned no `user_id`. `validateOpaqueSessionToken` returns `{ userId?: string }` straight from the response body, so a session that has not verified a user factor yet is a live case — `CreateSession` issues the `__nextgen_session` cookie before any factor exists, and `GetMySession` validates only token id + expiry. Route handlers and `getAuth()` callers received a synthetic identity matching no real user. A session without a user id is now unauthenticated — and its cookie is deliberately **kept**: the unauthenticated path sweeps `__nextgen*` cookies to stop browsers replaying dead credentials, but a session the backend just confirmed is not dead, and deleting it would orphan an in-progress login flow. Only dead credentials (non-2xx from `/sessions/me`, failed JWTs) are cleared. **`flow_definitions` — swallowed validation error.** `defs[i], err = domain.NewFlowDefinition(...)` was the last statement in the loop body, so the error was assigned, never read, and `return defs, nil` handed back an invalid definition with a nil error — `defs[i]` stayed nil, which would have panicked at seed time in `createDefaultLoginFlowDefinitions`. The three sibling `err` checks in the same function are all handled; this one was not. **`zotel` — dropped exporter error.** `readerOption, err := cfg.metrics(ctx)` was never checked, letting a misconfigured exporter start with metrics silently disabled. Now consistent with the tracer/logger siblings in `NewOtelMetrics`, which already fail fast. Default config cannot newly fail — the `ExporterTypeAuto` guard returns no reader and no error when nothing is configured. **`apps/cli` tests — superfluous argument.** `buildFlow` takes one parameter, so `buildFlow("password", ["email"])` bound `fields` to the string `"password"`; `[...fields, "password"]` then spread it into `["p","a","s","s","w","o","r","d","password"]`. The tests passed while validating a flow nobody intended. TypeScript did not catch it because `apps/cli/tsconfig.json` has `"include": []` — the vacuous-typecheck gap from the #698 audit, still open for the CLI. ## Validation - `moon run sdk-nuxt:test` — 98 passed (4 files), including three tests pinning the opaque-token path: user-bound session authenticates, anonymous session is unauthenticated **and keeps its cookie**, dead session has its cookie cleared. - Regression checks by reverting each fix: without the auth fix, the anonymous test fails with exactly `userId: "unknown"`; without the sweep gate, the cookie-preservation assertion fails while the dead-token sweep still passes. Both tests pin real behaviour rather than passing vacuously. - `moon run cli:test` — 963 passed (105 files). - `moon run sdk-nuxt:lint sdk-nuxt:typecheck cli:lint` — clean. - `go build ./...`, `go vet` on both touched packages, and `go test` over `flow_definitions`, `zotel`, and `internal/domain` — all pass. ## Release notes / changeset Changeset added: [`.changeset/reject-anonymous-session-identity.md`](.changeset/reject-anonymous-session-identity.md), listing `@zitadel/sdk-nuxt` and `@zitadel/server` at `patch`. Per the [decision table](.changeset/README.md#decision-table) this changes shipped product behaviour on both the SDK and the server, so a real changeset is required. Verified with `changeset status --since origin/main`. ## Notes Deliberately **not** included, so they can be judged separately: - **`internal/domain/session.go` `State()` ordering.** The finding is accurate — a session with no factors and a past `ExpiresAt` reports `building` rather than `expired`, reachable once `SessionAnonymousTTL` elapses, and visible in the session-widget chrome via `/sessions/me`. But `State()` has exactly one consumer (`sessionToAPI`), so this is presentation, not an authz gate — `validateSessionToken` enforces expiry independently. The AI's suggested reorder is unsafe as written: `NewSession()` leaves `ExpiresAt` at its zero value, so hoisting the expiry check makes every un-persisted session report `Expired`. A correct fix needs an `!s.ExpiresAt.IsZero()` guard and belongs in its own PR. A natural companion in that PR: the middleware could authenticate on `state === "active" && user_id` instead of treating `user_id` presence as a proxy for active — today the proxy holds (users bind at exchange), but "authenticate only what `State()` calls active" is the stronger invariant. - **`middleware.ts` bearer-token fallback.** A Bearer token that fails JWT verification still falls through to opaque cookie validation, contradicting the file's own "Bearer takes explicit precedence" comment. Not an escalation — the cookie is the caller's own credential, re-validated server-side — so it is hardening rather than a fix. - **`docs/design/decisions.md:48`** still says "template name" where D8 dropped template in favour of schema. Correct but cosmetic, and `docs:` work does not belong in a `fix:` PR. **One finding was dismissed as wrong.** The AI findings panel reports `new(s.ID)` / `new(s.ExpiresAt)` in `internal/domain/session.go` as allocating zero values, claiming the session token always encodes an empty `SessionID`. Go 1.26 extended `new` to accept a value expression, and this repo is on it (`go.mod: go 1.26`, toolchain go1.26.2) — `new(s.ID)` is exactly `gu.Ptr(s.ID)`, and the idiom is already used at `internal/api/session_test.go:24`. Verified directly with a scratch program. The model behind that panel appears to predate the language change, so Go pointer findings from it are worth confirming before acting on; the same finding was also emitted twice for one file. The `/security/code-scanning` tab itself is clean — all six alerts dismissed or fixed, none open.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Eleven of the repo's
typechecktasks were vacuous: they ran plaintsc --noEmitagainst a solution-style tsconfig ("files": []+references), and non-build-mode tsc ignores project references — so the checked program was empty and the task stayed green no matter what the code said. Probe-verified by droppingconst bad: number = "x"into components, sdk-core, api, and cli: all four passed; a real-program control (sdk-vue) correctly failed.tsc --build tsconfig.json(the pattern storybook already used): api, api-mock, components, config, design-tokens, sdk-next, shared-component-styles, testing, ui-react. Build mode walks the references and emits the per-file declarations that project-reference consumers already expect (the accommodation documented in components' tsdown config).apps/cli(~280 errors) andpackages/sdk-core(~600). Their real programs don't compile yet — a mix of genuine latent errors and a structural issue: their nodenext spec programs pull sibling-package sources in via the@zitadel/sourcecustom condition and compile them under foreign module settings (orval's extensionless imports explode under nodenext). That untangling is a design decision (fix the source condition, the module settings, or the generated-import style) and is tracked as a follow-up; both moon.yml files carry a KNOWN-VACUOUS marker.scripts/check-typecheck-programs.mjs(newworkspace:check-typechecktask,runInCI: true) statically re-derives the audit — resolves each typecheck command, finds the tsconfig it loads, and fails on any non-build tsc against a solution-style config. It also fails when the allowlist goes stale (an allowlisted project that starts checking a real program must be removed). Negative-tested: reverting one flip makes it exit 1 with a precise message.action.kind(values from the flow-definition authority);vitest.config.tsdeliberately left out of the spec program — its types trip over pnpm's duplicate vitest instances (split@types/nodepeer graph), and the file is exercised by vitest itself on every run.*.test.tsfiles sat in the lib project (exclude only covered*.spec.ts) — split fixed, so lib emits no test declarations and downstream references stop compiling config's tests; lib include gained the JSON dirssrc/actually imports (defaults/**,meta-schemas/); six latent test errors fixed (two unsound casts, three unchecked index accesses, one type predicate invalidated by literal-typedgoRefs).validateHandlenull-guard hoisted (two possibly-null accesses), mock-call index assertions made explicit.onKeyDownparameter annotation in the Add-user combobox — this fails typecheck on pristinemainin a clean store (from feat(console): schema-driven Add user drawer #673); CI is presumably green because task caching keys on inputs that don't includenode_modules. Fixed here since it blocks any honest gate run.Validation
tsc --build --force.kinds are render-neutral — config (122), testing (57).moon ci :lint :typecheck :build :test— 107 tasks green, including the newworkspace:check-typecheck.pnpm install --frozen-lockfilepasses — zero dependency changes (an earlier@types/nodeoverride experiment was abandoned after it disturbed the existing security-override block; the final change touches no dependencies).Release notes / changeset
No changeset required — no shipped behavior changed (CI/build wiring, tsconfig project hygiene, and test-only fixes; the one
@zitadel/testingsource touch reorders a validation guard with identical outcomes, and the console annotation is type-only).Notes
apps/cliandpackages/sdk-corecompile as real programs, then delete them fromKNOWN_VACUOUS— the guard enforces the removal the moment they go green.console:typecheckstayed green in CI while failing on a clean store — if remote task caching is keying pastnode_modules, other type drift can hide the same way.