fix(root): a job that installs nothing is checked for every program it starts - #1864
mobeenabdullah wants to merge 5 commits into
Conversation
|
@codex review |
|
Warning Review limit reachedNext included review available in 58 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (8)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3ccd074aee
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
@nextlyhq/adapter-drizzle
@nextlyhq/adapter-mysql
@nextlyhq/adapter-postgres
@nextlyhq/adapter-sqlite
@nextlyhq/admin
@nextlyhq/admin-css
@nextlyhq/blocks-engine
@nextlyhq/blocks-react
@nextlyhq/builder
create-nextly-app
@nextlyhq/eslint-plugin
nextly
@nextlyhq/plugin-form-builder
@nextlyhq/plugin-mcp
@nextlyhq/plugin-page-builder
@nextlyhq/plugin-sdk
@nextlyhq/plugin-seo
@nextlyhq/storage-s3
@nextlyhq/storage-uploadthing
@nextlyhq/storage-vercel-blob
@nextlyhq/ui
commit: |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b864ad3420
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f5f72de937
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
f5f72de to
7e27138
Compare
|
@codex review |
mobeenabdullah
changed the title
7e27138 to
862a123
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 862a123775
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
github-actions
Bot
added
type: docs
…d provide The "About line and topics" job in repo-metadata.yml has failed on every scheduled run since 2026-09-11, tracked in #1799. The About line and the topics were never wrong: the job crashes with ERR_MODULE_NOT_FOUND before it examines either. That job runs straight after checkout with no install, on purpose. check-repo-metadata.mjs imported the retired-category patterns from check-docs-claims.mjs, and #1767 gave that module an `@mdx-js/mdx` import, while its own import of check-docs-compile.mjs brings `js-yaml` too. ES imports evaluate eagerly, so importing two regexes loaded an MDX compiler the job has no way to resolve. The first failure ran an hour after #1767 merged, and nothing on that pull request could have seen it: the job runs on a schedule only, and pull-request CI installs dependencies before anything else runs. The patterns and their classifier move, verbatim, into retired-category.mjs, which imports nothing. The metadata check imports it directly; the docs-claims check imports and re-exports it, so there is still exactly one definition and every existing importer keeps its path. no-install-jobs.test.mjs holds the property from now on, in pull-request CI. It derives the jobs that install nothing from the workflow files themselves — today the CI gate and this check — and walks each script's import graph with TypeScript's parser, failing on any module Node alone cannot load, and on any import() it cannot follow rather than passing over it. Deriving that population exposed a blind spot in workflow-run-blocks.mjs: a step written `- run: ...` on the list item's own line was invisible to it. No workflow uses that form today, but a job written that way would have passed the guard unexamined. It is read now, with a block scalar bounded at the key's column rather than the dash's, so the step's own `env:` is not swallowed. Verified by reproducing the job faithfully — the whole scripts/ directory with no node_modules anywhere above it, under a schedule event — which exits 1 on main and 0 here; and by running every new test against the wrong implementation, including the exact original defect, which the guard fails naming both packages.
… refuses the rest The guard now reads workflows and local actions with js-yaml, the parser github-yaml-parses.test.mjs already applies to the same files, so a folded, quoted, flow-style or continued run: is the string GitHub runs. - Steps are read in run order and only up to the job's first install that can release it: unconditional, in the repository root, doing nothing else. - Composite actions are expanded in place; a JavaScript action's pre and post are placed at the job's start and end. - Scripts are split into commands by a new shell-commands.mjs, and a node command's arguments are read with Node's grammar, so an option's value is not taken for the script and a preloaded module is walked too. - working-directory is resolved from the step, the job or the workflow. - The module walk follows require, require.resolve, createRequire bindings and import.meta.resolve, resolving a relative require as require does. - Whatever the reader cannot settle is refused with the step and a reason. The text reader's additions are reverted: nothing uses them now.
…ns, env files and node.exe - A step's shell is its own, else the job's or the workflow's defaults.run.shell, and a shell that runs Node refuses the step. A composite action's step keeps the shell it names. - A condition or continue-on-error on the step using a local action binds every step inside it, so an install there no longer ends the checked stretch. - An install ends the stretch only when it runs in the repository root, with job and workflow default directories applied. - --env-file and --env-file-if-exists refuse the command, since the file can set NODE_OPTIONS. - node.exe and nodejs.exe count as Node, by name or by Windows path, and when named inside other text.
… and five more gaps close - The guard's TypeScript walk is replaced by @nextlyhq/module-specifiers, now a root devDependency. The reader gains what the guard needed: a require function createRequire returned (only when createRequire comes from module or node:module), require.resolve and import.meta.resolve, and a resolution label on every runtime reference, which is how a relative specifier is followed the way Node follows it. - An install ends the checked stretch only with options known to leave a full install, so --lockfile-only, --dry-run and --prod keep it checked. - A JavaScript action's post is read where the step using it is. - A step's shell is Node in every spelling a command has. - A step using a local action passes its environment into it. - The tokenizer keeps < input files, and a shell fed a script that way has the script read.
…ader cannot parse The shared module reader recognises a created require called where it is made, and labels every runtime reference with whether it runs the module or only finds it. The guard walks only what runs, still reports a package that is only resolved, and follows a relative require through Node's documented CommonJS search, package.json main included, held to require.resolve on disk. A step's script is read only under bash, sh, zsh or dash. A Windows runner's default PowerShell, any other named shell, and a runner whose runs-on names no single system are refused, with the remedy in the reason.
862a123 to
70f0af3
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 70f0af360c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const INERT_SUBCOMMANDS = new Set([ | ||
| "install", "i", "ci", "add", "audit", "view", "info", "show", "whoami", "config", "get", "set", | ||
| "ls", "list", "why", "outdated", "store", "bin", "root", "prefix", "cache", "dist-tag", "ping", | ||
| "help", "enable", "prepare", "use", |
There was a problem hiding this comment.
When a pre-install step runs pnpm prepare, the root package's prepare script (husky, at package.json:86) executes, but this shared allowlist classifies prepare as inert for every package manager, so no refusal or start is emitted; on a fresh checkout this can fail because husky is not installed. With the pinned pnpm 9.0.0, pnpm help run says “Runs a defined package script,” pnpm help prepare returns “No results,” and an isolated package confirms that pnpm prepare invokes its prepare script; make these subcommands manager-specific or remove prepare for pnpm.
Useful? React with 👍 / 👎.
| ts.isImportDeclaration(node) && | ||
| ts.isStringLiteralLike(node.moduleSpecifier) && | ||
| NODE_MODULE_SPECIFIERS.has(node.moduleSpecifier.text) | ||
| ) { | ||
| recordImportedBindings(node, bindings); |
There was a problem hiding this comment.
When a dependency-free entry uses const { createRequire } = await import("node:module") and then calls createRequire(import.meta.url)("js-yaml"), this collector records only the builtin dynamic import; it never marks createRequire as a factory, so nonBuiltinImports returns clean while Node attempts to load the missing package. This loader form is already recognized elsewhere at packages/ui/src/layering.test.ts:898-905; extend the shared reader's binding discovery rather than adding another guard-specific scan.
AGENTS.md reference: AGENTS.md:L299-L302
Useful? React with 👍 / 👎.
|
|
||
| /** Node's options that take no value. */ | ||
| const NODE_FLAGS = new Set([ | ||
| "-c", "--check", "--enable-source-maps", "--expose-gc", "--frozen-intrinsics", "--no-addons", |
There was a problem hiding this comment.
When a no-install step runs node --check scripts/a.mjs and that file imports an npm package, this flag classification causes readNodeCommand to register a normal start and startOffenders to walk its imports, failing the guard even though Node never resolves them. The inspected Node 20.20.2 node --help defines -c, --check as “syntax check script without executing,” and node --check exits 0 for a file importing an absent package; unlike the earlier option-argument gap, this option is recognized but its non-executing semantics are lost. Treat check mode as non-loading while still validating explicit preloads, which Node does execute.
Useful? React with 👍 / 👎.
| * What the word after each redirection operator is. `<` names the file a command reads as its | ||
| * input, which a shell given no script file runs; every other operator is followed by a file | ||
| * written to or duplicated, which runs nothing. | ||
| */ |
There was a problem hiding this comment.
When a Bash step runs bash <> scripts/run.sh, Bash opens that file on standard input for reading and writing and executes its contents, but this map classifies only < as an input source; the tokenizer drops the <> target as a generic file, so readShellCommand sees no script and Node commands inside it are omitted. Fresh evidence beyond the earlier < fix is that <> remains recognized by REDIRECTION but absent here; preserve the redirected descriptor and treat <> as input when it targets fd 0, including the implicit-fd form.
Useful? React with 👍 / 👎.
| defaults, | ||
| defaultShell: job.defaults?.run?.shell ?? workflow.defaults?.run?.shell, | ||
| runnerShell: runnerDefaultShell(job["runs-on"]), | ||
| env: [workflow.env, job.env], |
There was a problem hiding this comment.
When a container job sets jobs.<job_id>.container.env.NODE_OPTIONS, GitHub supplies that variable to every step in the container, but this scope includes only workflow-level and job-level env. A builtin-only node scripts/check.mjs is consequently recorded without its container-provided preload and passes the guard even though Node attempts to load the missing package before the entry; include job.container?.env in the effective environment.
Useful? React with 👍 / 👎.
| for (const specifier of parsed.preloads) { | ||
| const path = /^[./]/.test(specifier) ? repositoryPath(specifier, state.cwd, "preload") : null; | ||
| if (path !== null && "refusal" in path) return refuse(state, path.refusal); | ||
| preloads.push(path === null ? { package: specifier } : { path: path.path }); |
There was a problem hiding this comment.
When a no-install command uses node -r ./tools/setup scripts/a.js and the repository contains tools/setup.js, Node loads that extension-added file, but this code records the path exactly as written and startOffenders reports tools/setup: cannot be read. The inspected Node 20.20.2 help identifies -r, --require as a “CommonJS module to preload,” and the runtime resolves the extensionless form successfully; preserve the preload option's resolver and reuse the existing CommonJS target resolution instead of treating --require like --import and --loader.
AGENTS.md reference: AGENTS.md:L299-L302
Useful? React with 👍 / 👎.
1 participant
The fix for
main's red scheduled metadata job is #1879. This pull request is the guard that stops that failure from coming back, together with the shared module reader it now builds on.Guard, so this cannot recur silently
scripts/no-install-jobs.test.mjsruns in pull-request CI (pnpm test:scripts, in thecijob). It derives, from the workflow files themselves, every program a job starts before it has installed dependencies. It then fails on any module in that program's import graph that Node alone cannot load. Today that population isci.ymlgate→scripts/ci-gate.mjsandrepo-metadata.ymlmetadata→scripts/check-repo-metadata.mjs.A guard that reads a workflow differently from GitHub passes over exactly what it exists to catch, so it reads the way GitHub does:
github-yaml-parses.test.mjsalready applies to these files, so folded, quoted, flow-style and continuedrun:values are the string GitHub runsif:, nocontinue-on-error, no package argument, no second command, and run in the repository root once a job or workflow default directory is applied. Only options known to leave a full install count, so--lockfile-only,--dry-runand--prodkeep the job checked. An install inside a local action counts only when the step using the action has no condition of its ownpreruns at job start; itsmainandpostare read where its step is, since apostruns only for a step that ran and even after a failed installscripts/shell-commands.mjshandles quoting, comments, continuations, operators, redirections, here-documents and command substitution. An expansion is marked non-literal rather than guessed. A shell fed its script through<has that file readshell, else the job's or workflow'sdefaults.run.shell, else the runner's default: bash for anubuntu-ormacos-image or alinuxormacOSlabel, PowerShell for Windows. Only bash, sh, zsh and dash are read, since that is the grammar the tokenizer implements. A shell that is Node refuses the step as inline code, in every spelling a command has:nodejs, any case, a path,.exe. A composite action's step names its own, so a job default does not reach itnodearguments--, value options that consume the next word (checked against the binary), and=forms. The modules-r/--import/--loaderpreload are walked too.--env-fileis refused, since the file can setNODE_OPTIONS.node.execounts as Nodeworking-directoryfrom the step, the job's defaults or the workflow's@nextlyhq/module-specifiers, the repository's one reader for what a source file loads. It now also reads a require functioncreateRequirereturned, whether bound to a name or called where it is made,require.resolve()andimport.meta.resolve(), and labels each runtime reference with the resolver that finds it and whether it runs the module. The walk follows only what runs, still reports a package that is only resolved, and follows a relative specifier exactly as written for an import and through Node's documented CommonJS search,package.jsonmainincluded, for a requireWhen it cannot settle something, it refuses, naming the step and the reason, rather than passing over it:
nodeargument that is not literal, an unknown option, or inline code or--run;nodeanywhere else;NODE_OPTIONS;runs-onnames no single system;cdbeforenode;review-bot-gh.sh, and a control proves it is load-bearing.The earlier additions to
workflow-run-blocks.mjsare reverted, since this check no longer uses that module.Verification
Every test was run against a wrong implementation of what it guards: 97 of 97 were caught, each by the test named for it. A sample:
scripts/check-repo-metadata.mjsreads the steps before a job's install, and none after itif:ends the checked stretchdoes not end it at an install with a conditionchangesjob controlreads a folded script, and the real ci.yml folded-command controlnodeoption treated as a flag--conditions,-C, preload and refusal casesworking-directoryignoredrequire()ignoredreads require, require.resolve, …andfollows a relative require …finds node in a command substitution …refuses a script the job's / the workflow's default shell runs as Node codedoes not end it at an install inside an action whose step has a conditiondoes not end it at an install in a job's default directory--env-fileacceptednode.exenot recognisedstarts Windows' node.exe, by name or by path, from bash on a Windows runnercreateRequirereturned ignoreda require function createRequire returnedcases, and the guard'sfollows a relative require through the forms require triespostread at the job's endreads a JavaScript action's post when the step using it comes before the installdoes not walk a file require.resolve / import.meta.resolve only finds, with its loading control, andreports a package … looks forpackage.jsonmainignored, or any step of Node's CommonJS search dropped or reorderedthe file a relative require loads: 20 specifiers on disk, each walked by the guard and resolved by Node's ownrequire.resolve, which agree on Node 20.20.2, 22.18.0, 24.17.0 and 26.8.2a created require called where it is made, and the guard'srefuses a Windows runner's default PowerShell …with its bash control, and the named-shell, runner-label and composite casestest:scripts0 (43 files, 1543 tests) ·@nextlyhq/module-specifiers72 tests, check-types and lint 0 · the admin, builder and ui suites that consume the reader pass unchanged ·lint:scripts0 ·check:comments0 · changeset gate 0 · fallowpass.One changeset for the release group, because
@nextlyhq/module-specifierschanges.The fix itself, and the closing of #1799, are in #1879.