Skip to content

#1289 Remove shelled nmr calls from package manifests - #1291

Merged
williamthorsen merged 5 commits into
mainfrom
1289
Aug 11, 2026
Merged

#1289 Remove shelled nmr calls from package manifests#1291
williamthorsen merged 5 commits into
mainfrom
1289

Conversation

@williamthorsen

@williamthorsen williamthorsen commented Aug 11, 2026

Copy link
Copy Markdown
Owner

What

nmr build no longer reaches nmr through a shell, so a failing package build reports the step that failed instead of the whole nested subtree, and interrupting a run stops what would have followed. Five packages had a build override doing that: kb, lifecycle, mcp, and run-core restated a default nmr already supplies and now resolve build to its built-in ['compile'], and codeassembly's two post-compile steps move into its build:post hook. The warning nmr printed for each, five on every build, goes with them.

A root test keeps the pattern out: It fails when any manifest in the repo, the monorepo root's included, declares a script reaching nmr. It catches the forms nmr's own warning misses, npx nmr and pnpm --recursive exec nmr among them, and exempts the npm lifecycle names along with root's bootstrap.

Why

Debugging a failed build meant reading a whole nested subtree to find the one step that failed, and interrupting the run left the rest of the chain still to come. The warnings that flagged this appeared five at a time on every nmr build, which is how a warning channel stops being read. Nothing needed the overrides in the first place: four of the five restated a default nmr already supplies.

Details

🧪 Tests

  • scripts/__tests__/package-scripts.unit.test.ts reads the monorepo root's manifest and every workspace package's, and fails on a script value reaching nmr as a whole token. The root manifest is covered because nmr resolves it as a tier-3 override the way it resolves a package's: at root scope anchorDir is the monorepo root, which is how a root hook such as ci:post runs at all.
  • The detector, findShelledNmrScripts, carries direct cases of its own: each runner form it must report (nmr compile, pnpm exec nmr, pnpm --filter x exec nmr, pnpm --recursive exec nmr, npx nmr), each form it must pass over (nmr-compile, nmr-fmt --check, an nmr/bin path segment), and the exempt names. An edit that stopped the pattern matching fails the suite rather than passing it green.
  • Exemptions are the npm lifecycle names in every manifest, since nmr resolves none of them as a command and prepublishOnly: "nmr build" is the upstream-sanctioned pattern, plus bootstrap in the root manifest, which builds and deploys from a fresh checkout where the bare nmr binary is not yet on the path.
  • Both root suites take their package set from getWorkspacePackageDirs(findMonorepoRoot(...)) in @williamthorsen/nmr/workspace, replacing a hardcoded packages/ path and a readdirSync walk in each. pnpm-workspace.yaml is now the authority on what counts as a package, and a directory holding no manifest is skipped rather than crashing the read.

⚙️ Tooling

  • kb, lifecycle, mcp, and run-core drop "build": "nmr compile" and resolve build to nmr's built-in ['compile']. mcp and run-core are left with no scripts block; kb and lifecycle keep prepublishOnly. Publication is unaffected either way, since the reusable publish workflow runs release-kit publish rather than a recursive build.
  • codeassembly moves tsx scripts/bundle-skill-helpers.ts and tsx scripts/copy-content.ts out of its build script and into build:post, ahead of rdy compile. Both were already post-compile work, so the hook runs them where they ran before: rdy verify, compile, bundle, copy, rdy compile. A hook rather than a step list, because workspaceScripts has no per-package keying and a tier-2 build list would impose this chain on every package.
  • fleet's ":" build stays, an intentional skip whose removal would start compiling a tsx-run server, as do factory's and foreman's vite builds, which invoke no nmr.
  • .config/nmr.config.ts needs no entry, contrary to what the warning suggests: resolveScript returns on the package.json hit before reaching the registry, so a config entry stays shadowed while an override remains. Reported upstream as Shelled-nmr diagnostic reports the wrong remedy, no origin, and only half the crossings node-monorepo-tools#656.

Closes #1289

Drops the `build` script from `kb`, `lifecycle`, `mcp`, and `run-core`, each of which restated nmr's built-in workspace default (`['compile']`) as the shell string `nmr compile`. `nmr build` no longer warns that those four run nmr behind a shell, where a quiet failure surrenders the whole nested subtree instead of the failing step.
Moves `bundle-skill-helpers.ts` and `copy-content.ts` out of the `codeassembly` package's `build` script and into its `build:post` hook, ahead of `rdy compile`. The package resolves `build` to nmr's built-in default, so `nmr build` no longer warns that it runs nmr behind a shell. The build sequence is unchanged: both steps already ran after the compile.
Adds a root test that fails when a workspace `package.json` declares a script reaching `nmr` in command position, directly or behind `pnpm exec`. The npm lifecycle hooks are exempt, since nmr resolves none of them as a command and `prepublishOnly: "nmr build"` is the established pattern; so are the leaf bins `nmr-compile` and `nmr-fmt`, neither of which starts a nested run.
Replaces the hardcoded `packages/` path and `readdirSync` walk in both root suites with `getWorkspacePackageDirs(findMonorepoRoot(...))` from `@williamthorsen/nmr/workspace`. `pnpm-workspace.yaml` is now the authority, so a pattern added there is covered without editing either suite; a directory under `packages/` holding no manifest is skipped rather than crashing the read; and the discovery lives in one published helper instead of two copies.
…cases

Extends the guard to the monorepo root's `package.json`, which nmr resolves as a tier-3 override the way it resolves a package's, and exempts `bootstrap` there by name so the deliberate pnpm-fronted call is recorded where a reader meets it. Root hooks such as `ci:post` are now covered.

Widens `shelledNmrPattern` to match `nmr` as a whole token, so `npx nmr`, `pnpm --filter x exec nmr`, and `pnpm --recursive exec nmr` are reported. nmr's own warning inspects only a command's first token and reports none of those, leaving both mechanisms blind to them.

Adds direct cases over `findShelledNmrScripts` covering each runner form, the leaf bins, and the exempt names, so an edit that stopped the pattern matching fails the suite instead of passing it. `WorkspaceManifest.name` becomes `GuardedManifest.manifestPath`, which no longer collides with the sibling suite's package-name `name`.
@williamthorsen williamthorsen added the tooling Development tools label Aug 11, 2026
@github-actions

Copy link
Copy Markdown

Dependency audit

Production dependency audit passed.

@williamthorsen williamthorsen self-assigned this Aug 11, 2026
@williamthorsen
williamthorsen marked this pull request as ready for review August 11, 2026 16:44
@williamthorsen williamthorsen changed the title #1289 Remove the shelled nmr calls from the package manifests and guard against their return #1289 Remove shelled nmr calls from package manifests Aug 11, 2026
@williamthorsen
williamthorsen merged commit 8cc3027 into main Aug 11, 2026
3 checks passed
@williamthorsen
williamthorsen deleted the 1289 branch August 11, 2026 16:52
williamthorsen added a commit that referenced this pull request Aug 13, 2026
…e-v0.3.0 codeassembly-v0.9.0 factory-v0.2.6 codeassembly-mcp-v0.2.5

codeassembly-v0.9.0
- #1288 feat: State mutability and name the lede reader at the artifact type lists (#1303)
- #1285 fix: Require plain bullets for checklists on both update-jira-ticket paths (#1302)
- deps: Upgrade all deps to latest version
- #1290 tooling: Upgrade eslint-config-typescript to 10 and complete manifest metadata (#1301)
- #1294 feat: Specify artifact mutability and its sanctioned exceptions (#1297)
- #1293 fix: Present and classify discretionary findings as actionable (#1295)
- #1289 tooling: Remove shelled nmr calls from package manifests (#1291)
- #1280 feat: Add a refresh path that edits an existing AGENTS.md instead of redrafting it (#1287)
- #1055 feat: Replace the lede doctrine with corpus-derived guidance and retire the changelog-writer (#1286)
- deps: Upgrade all deps to latest version
- #1264 refactor: Consolidate error-message extraction on toolbelt.errors' describeError (#1284)
- #1263 refactor: Retire the deferred-lint mechanism and adopt toolbelt.errors in agents (#1279)
- #1262 refactor: Hoist unreadable for-of expressions and promote the lint rule to an error (#1276)
- #1267 fix: Extend the no-hard-wrapping instruction to every GitHub composer (#1275)
- #1177 fix: Rewrite links, tokens, and template variables in a Markdown file support entry (#1273)
- #1176 feat: Add a ticketing-preferences hook to the skills that split and create tickets (#1268)
- #1252 fix: Keep a skill's own sections out of injected guidance (#1266)
- #1260 refactor: Fix lint and retire rule deferrals (#1265)
- #1247 feat: Add a personal tooling-preferences rulebook (#1257)
- #1251 feat: Let a rulebook declare that a guidance hook is how it is reached (#1259)
- #1246 feat: Guard home-domain writes and record their provenance (#1254)
- #1175 feat: Link new tickets to their blockers and parents (#1249)

factory-v0.2.6
- deps: Upgrade all deps to latest version
- #1290 tooling: Upgrade eslint-config-typescript to 10 and complete manifest metadata (#1301)
- deps: Upgrade all deps to latest version
- deps: Upgrade all deps to latest version
- #1264 refactor: Consolidate error-message extraction on toolbelt.errors' describeError (#1284)

kb-v0.6.0
- deps: Upgrade all deps to latest version
- #1290 tooling: Upgrade eslint-config-typescript to 10 and complete manifest metadata (#1301)
- #1289 tooling: Remove shelled nmr calls from package manifests (#1291)
- #1264 refactor: Consolidate error-message extraction on toolbelt.errors' describeError (#1284)
- #1261 feat: Attach causes to kb's loader errors and retire its lint deferral (#1272)
- #1260 refactor: Fix lint and retire rule deferrals (#1265)

codeassembly-lifecycle-v0.4.0
- #1290 tooling: Upgrade eslint-config-typescript to 10 and complete manifest metadata (#1301)
- #1289 tooling: Remove shelled nmr calls from package manifests (#1291)
- #1261 feat: Attach causes to kb's loader errors and retire its lint deferral (#1272)
- #1260 refactor: Fix lint and retire rule deferrals (#1265)

codeassembly-mcp-v0.2.5
- deps: Upgrade all deps to latest version
- #1290 tooling: Upgrade eslint-config-typescript to 10 and complete manifest metadata (#1301)
- #1289 tooling: Remove shelled nmr calls from package manifests (#1291)
- #1264 refactor: Consolidate error-message extraction on toolbelt.errors' describeError (#1284)

codeassembly-run-core-v0.3.0
- deps: Upgrade all deps to latest version
- #1290 tooling: Upgrade eslint-config-typescript to 10 and complete manifest metadata (#1301)
- #1289 tooling: Remove shelled nmr calls from package manifests (#1291)
- #1264 refactor: Consolidate error-message extraction on toolbelt.errors' describeError (#1284)
- #1261 feat: Attach causes to kb's loader errors and retire its lint deferral (#1272)
- #1260 refactor: Fix lint and retire rule deferrals (#1265)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

tooling Development tools

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Package build scripts invoke nmr through a shell, so every nmr build warns

1 participant