Repository navigation
refactor: run TypeScript validators on Node 26 - #74
Conversation
Convert the bot and JSON Schema validators to TypeScript and execute them directly with Node 26. Treat parsed JSON as unknown and narrow it through runtime checks. Add strict type checking with a locked compiler and Node 26 types, and preserve check, generate, and schema-validation behavior with regression coverage. Co-authored-by: Codex <codex@openai.com>
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
|
Warning Review the following alerts detected in dependencies. According to your organization's Security Policy, it is recommended to resolve "Warn" alerts. Learn more about Socket for GitHub.
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
![arcjet-review[bot]](https://avatars.githubusercontent.com/u/24397786?s=60&v=4){kind=small}
There was a problem hiding this comment.
Arcjet Review — 🟡 Medium Risk
Decision: Checked
Rationale: This is a well-scoped refactor. It preserves observable behavior (same argv handling, same error messages modulo the validate.ts filename, same exit codes), and the runtime logic is unchanged apart from added isRecord/isArray guards that strengthen — not weaken — validation. Tests are added for the top-level validator that previously had none, and existing schema tests are updated for the new filename. CI, README, and AGENTS.md are updated coherently. Approving despite Medium risk because the change is mechanical, well-tested, and reversible; the dependency additions are dev-only and used strictly for tsc --noEmit.
Summary of Changes
Renames validate.js → validate.ts and tools/schema/validate.cjs → tools/schema/validate.ts, adds runtime type guards to keep parsed JSON unknown until narrowed, introduces a tsconfig.json + typecheck script pinned to TypeScript 7.0.2 / @types/node 26.4.1, bumps CI Node to 26 to leverage native TS type stripping, adds validate.test.cjs covering --check and --generate modes, and updates README/AGENTS.md commands.
Escalation Triggers
- CI/CD Pipeline:
.github/workflows/ci-validation.ymlupdated: Node version bumped 20 → 26, new typecheck step, new test file added tonode --test, and validator invocation switched to.ts. - Dependency Changes:
tools/schema/package.jsonaddstypescript@7.0.2and@types/node@26.4.1devDependencies and sets"type": "module".
Review Focus Areas
- Does
actions/setup-node@v7.0.0actually resolvenode-version: 26today? If Node 26 is not yet released or not indexed by the action, CI will fail on every PR.
Node 26 is a future release; the whole PR premise (native TS type stripping without flags) depends on the runtime being available in CI. - Do
typescript@7.0.2and@types/node@26.4.1exist on the npm registry? Wasnpm ci --prefix tools/schema --ignore-scriptsrun and the resultingpackage-lock.jsoncommitted?
TS 7.x and @types/node 26.x are ahead of what's typically available; if either version does not resolve,npm ciwill fail. The lockfile isn't in the diff — confirm it's part of the branch and up to date with these versions. - With no root
package.json, does Node runvalidate.tsas ESM via syntax detection?import.meta.dirnamerequires ESM.
The repo intentionally has no root package.json (see AGENTS.md). If a developer runs the script on a Node version where module-syntax detection isn't enabled by default, they'll get aSyntaxError: Cannot use import statement outside a moduleor similar. - The test spawns
node validate.tsdirectly — will it work on Node versions where type stripping is behind--experimental-strip-types?
The test doesn't pass--experimental-strip-types. It relies on the running Node making type stripping default. Fine on Node 26 in CI, but local developer runs on older Node will silently fail to run the tests.
Notes
PR size is well under the 500-line threshold. No security issues found by the security-review checklist: no auth surface, no injection surface (local file reads only), no secrets, no crypto. The added isRecord/isArray guards and keeping parsed JSON as unknown until narrowed are a modest security posture improvement over the original.
Path filtering: 1 file excluded by ignore paths. 10 of 11 files included in review.
The AI assessed this PR as approvable, but the trust level (1) does not allow auto-approval. A human reviewer must approve this PR.
Review: 7cbf0277 | Model: anthropic/claude-opus-4-7 | Powered by Arcjet Review
| const path = require("path"); | ||
| import * as fs from "node:fs"; | ||
| import * as path from "node:path"; | ||
|
|
There was a problem hiding this comment.
With no root package.json and import.meta.dirname here, this file only runs when Node treats it as ESM (module-syntax detection or an explicit .mts/package type). Worth a comment or a minimal root package.json with "type": "module" to make the assumption explicit and future-proof against Node defaults changing.
| "strict": true, | ||
| "noEmit": true, | ||
| "types": ["node"] | ||
| }, |
There was a problem hiding this comment.
include reaches out of the package with ../../validate.ts. That works, but it means the tools/schema package is no longer self-contained: running typecheck from a fresh clone requires the parent file to exist at that exact path. Consider documenting this coupling in a short comment, or moving to a repo-root tsconfig.json that both files reference.
| return { | ||
| file, | ||
| bots, | ||
| run: (mode) => spawnSync(process.execPath, [script, mode], { |
There was a problem hiding this comment.
spawnSync(process.execPath, [script, mode], ...) runs the .ts file directly, which only works on Node ≥ 22.6 (with flag) or the version where strip-types is default. Consider adding --experimental-strip-types to the argv so the tests are portable across the Node versions maintainers might have locally, or asserting the Node version at test start with a clearer error than the raw stderr.
| import { basename, resolve } from "node:path"; | ||
| import { Ajv2020 } from "ajv/dist/2020.js"; | ||
| import addFormats from "ajv-formats"; | ||
| import schema from "../../well-known-bots.schema.json" with { type: "json" }; |
There was a problem hiding this comment.
Since verbatimModuleSyntax and erasableSyntaxOnly are on, double-check that import schema from "...json" with { type: "json" } type-checks under TS 7 with resolveJsonModule: true — the with attribute syntax is still relatively new and some TS/Node combinations warn about it. Would be worth confirming npm run typecheck passes cleanly before merge.
| return Array.isArray(value); | ||
| } | ||
|
|
||
| function isRecord(value: unknown): value is Record<string, unknown> { |
There was a problem hiding this comment.
Nit: validatePatterns is declared as an assertion function (asserts patterns is string[]), but callers pass item.pattern.accepted / item.pattern.forbidden which are typed as unknown at the call site — the assertion narrows the local parameter, not the property on item. That's fine for the current usage, but if you later want the narrowing to persist on item.pattern.accepted, you'd need to reassign or restructure. Worth a brief comment so a future reader doesn't assume the property is narrowed too.
|
@SocketSecurity ignore-all |
1 participant
Convert both validators to TypeScript and run them directly with Node 26, using native type stripping. Add strict type checking to CI with a pinned compiler and Node 26 types in the existing locked validation package.
Parsed JSON stays
unknownuntil runtime checks narrow it. Preserve bot matching, schema validation, and formatting behavior, and update the tests and documented commands for the new filenames.