fix: Phase A workspace-architecture defects, round two - #37
Merged
Merged
Conversation
resolveWorkspacePackageDirs previously matched nothing for a "packages" glob written with a "./" prefix or a "."/".." segment, since the matcher walked pnpm-workspace.yaml's raw, unnormalised segments; segments are now normalised the way path.posix.normalize would before matching, and a positive glob that still resolves to zero directories throws, naming the pattern, rather than silently linting nothing. listRealSubdirectories now excludes bower_components as well as node_modules, matching the installed pnpm binary's own exclusion list (the header comment's "resolves through fast-glob" claim is corrected against that same binary, a compiled Rust executable with no such library in it). segmentToRegExp now takes a "]" immediately after "[" or a negation marker as a literal class member instead of an empty class, and honours a backslash as an escape for the character after it.
readDeclaredManifest previously treated a package.json with no usable "name" the same as one that fails to parse at all, so buildWorkspaceGraph dropped it from the graph entirely; pnpm allows a workspace package to omit "name", so such a package is now kept and keyed by its own relativeDir, letting its outgoing dependencies still be checked instead of silently skipped. readDeclaredManifest also now wraps JSON.parse and names the manifest's own path in the thrown error, rather than surfacing a bare SyntaxError with no indication of which file. Adds manifestRelativeDir, the shared helper the three workspace-architecture rules use to confirm a linted manifest against the graph entry they resolved it from.
…graph entry no-uphill-dependency, no-dependency-cycle and package-name-mirrors-path each identified "self" purely by the manifest's own declared name, so a stale or duplicated copy of a real package's package.json elsewhere in the tree (a build output directory that copied it verbatim, say) was checked as that same real package: reporting edges the copy never declared, or double-reporting a real cycle once per copy. Each rule now also confirms, via manifestRelativeDir, that context.filename's own directory is the resolved graph entry's own relativeDir, and skips a mismatch instead of reporting it. no-uphill-dependency and no-dependency-cycle also now fall back to identifying self by directory when a manifest declares no name at all, matching how buildWorkspaceGraph now keys such a package, so its own dependencies are checked rather than silently skipped.
…segment
expectedPackageName's 'keep-group' strategy kept every segment of a
package's relativeDir, including container directories above a
group's own nested path. A group declared with a multi-segment path
(such as { path: 'packages/test' }) derived a name prefixed with
every one of those segments instead of just its own; it now keeps
only the group's own path/name segment (the last one, for a nested
path) ahead of the remaining segments, matching what a group with no
nested path already produced.
rankSkip.maxDistance accepted a negative or fractional value (a negative maxDistance flags every same-rank dependency as a rankSkip violation), isolatedGroups accepted a pair naming the same group twice (turning every intra-group dependency into a false violation), nameRanks patterns were never checked for validity until deriveRank compiled one deep inside a lint run, and rank/defaultRank/exemptRanks accepted a non-integer number. readWorkspaceArchitectureOptions and the JSON schema now reject each of these: maxDistance and every rank field require an integer (maxDistance also non-negative), an isolatedGroups pair must name two distinct groups, and every nameRanks pattern is compiled up front, naming the option and the pattern in the error rather than surfacing a bare "Invalid regular expression" later with no indication of which option produced it.
…ectory A positive "packages" glob matching zero directories (an empty group awaiting its first member) now resolves to no packages, the same as pnpm itself, instead of throwing and aborting every workspace-architecture rule. This was breaking real consumers whose workspace declares a glob over a group with no members yet (a freshly scaffolded vertical, a group holding only a README). Also simplifies segmentToRegExp's character-class close search: the "]" immediately after bodyStart is always searched for one position past bodyStart regardless of whether that position holds a literal "]", since indexOf can never match a character that is not there. Adds tests for consecutive leading ".." segments and a "]" immediately after a "^" negation marker.
An assertion function narrowing a caught unknown value back to Error in place, for the two call sites (JSON.parse in workspace-graph.ts, the RegExp constructor in workspace-options.ts) that only ever throw a real Error on failure. It asserts rather than renaming the value specifically so a re-thrown error's own "cause" can still reference the exact same identifier the catch clause bound, which this package's own preserve-caught-error lint rule requires.
…ss-package matching manifestRelativeDir now resolves both the workspace root and the linted file's own directory through fs.realpathSync before comparing. An explicit "root" option and ESLint's own context.filename can spell the identical real directory two different ways (a symlink anywhere on either path, macOS /tmp vs /private/tmp being the recurring real case), and a purely lexical relative() between the two spellings computed a path nowhere near the graph entry's own relativeDir, silently making every workspace-architecture rule report nothing for a correctly configured package. WorkspaceFs gains a realpathSync seam for this. deriveRank and deriveSlice's own 'namePrefix' branch now skip nameRanks and sliceByNamePrefix respectively for a package with no declared name at all (pnpm allows omitting it): both match a pattern against the package's own declared name, never against the relativeDir fallback collectCandidates uses as its graph identity key, so a nameless package falls straight through to its group's rank/slice instead of accidentally matching a pattern against its own directory path. readDeclaredManifest now narrows its caught JSON.parse error via assertIsError rather than a ternary String(error) fallback, and moves the manifest read itself outside the try so a real read failure (EACCES, a race that removes the file) is never misreported as a JSON parse error.
…'u' flag and cause validateRankRulePatterns now narrows its caught RegExp SyntaxError via assertIsError rather than a ternary String(error) fallback, matching readDeclaredManifest's own fix. Adds a test compiling a nameRanks pattern that is valid without the 'u' flag but invalid with it (an unnecessary "\-" escape), which deriveRank itself later compiles with that same flag, and a test asserting the thrown error's own "cause". isInteger drops its redundant "typeof value === 'number'" guard: Number.isInteger already returns false for any non-number input, so the guard could only ever agree with what it already decides.
…rrors-path
Each rule factory now takes a bundled deps object ({ loadGraph?, fs? })
instead of two trailing optional positional parameters, satisfying this
package's own prefer-options-object-param rule, and threads a WorkspaceFs
through to manifestRelativeDir for its realpath resolution.
package-name-mirrors-path no longer returns early for a manifest with no
declared "name" at all: it is looked up in the graph by its own
relativeDir, the same fallback buildWorkspaceGraph itself uses, and
reported under a new "missingName" message id, since a package that
declares no name plainly cannot mirror its path either.
Adds a regression test to each of the three rules for a nested object
with no name of its own resolving, via that same relativeDir fallback,
to the exact same graph entry as the real top-level manifest: the new
relativeDir confirmation had started masking the existing "nested real
name" tests, since a mismatched relativeDir alone was enough to skip a
nested object even with the top-level parent.type === 'Document' guard
bypassed.
…dling The groups.naming field's own three strategies (drop-group, keep-group, basename) now state their exact segment derivation, since this PR changes keep-group's own semantics (only the group's own last path segment is kept, not every container segment above it). Also documents that an empty positive packages glob resolves to no packages rather than throwing, that nameRanks skips a nameless package entirely, and that package-name-mirrors-path reports one too.
…y testable readDeclaredManifest and validateRankRulePatterns each pass a template-literal context string to assertIsError, but assertIsError's own throw is unreachable at both call sites: JSON.parse and the RegExp constructor only ever throw a real Error, never a non-Error value, so no test exercising the real call path can observe which context string was passed. Extracts each into its own named function (jsonParseContext, regExpConstructorContext) in workspace-errors.ts and asserts their exact output directly, the same "Unreachable, tested directly" treatment this package already gives requireChar/findDependencyEntry/findGroupSpec/last.
Mearman
marked this pull request as ready for review
September 26, 2026 22:51
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. |
|
🎉 This PR is included in version 2.22.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
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
1 participant
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.
Fixes the defects an adversarial review found in the workspace-architecture rules from PR #35, on a fresh branch off main since #35 had already merged and released (2.21.1) by the time this work started.
A second adversarial review then found six further defects in this PR's own diff, fixed in the later commits below:
workspace-glob.ts:resolveWorkspacePackageDirsno longer throws when a positivepackagesglob currently matches no directory (an empty group awaiting its first member); it resolves to no packages instead, the same as pnpm itself. The throw was breaking real consumers, including exchange-platform's ownpnpm-workspace.yamlglobs over groups that today hold only a README.segmentToRegExp's character-class close search is also simplified (indexOffrombodyStart + 1unconditionally, since it can never match a character that isn't there).workspace-graph.ts:manifestRelativeDirnow comparesrootand the linted file's own directory throughfs.realpathSyncbefore taking their relative path, so an explicitrootoption and ESLint'scontext.filenamespelling the same real directory two different ways (a symlink, macOS/tmpvs/private/tmp) no longer makes every workspace-architecture rule silently report nothing.deriveRank'snameRanksandderiveSlice'snamePrefixbranch now match against a package's genuine declarednameonly, never against therelativeDirfallback its graph identity key uses when it declares none.readDeclaredManifestno longer wraps thefs.readFileSynccall in the sametryasJSON.parse, so a real read failure is never misreported as a JSON parse error.package-name-mirrors-path.ts: a package that declares nonameat all is now reported (a newmissingNamemessage), rather than silently skipped, since it plainly cannot mirror its path either.workspace-options.ts/workspace-graph.ts:readDeclaredManifestandvalidateRankRulePatternsnarrow their caught errors through a new sharedassertIsErrorguard and thread the original error throughcause, instead of aString(error)fallback for a non-Error throw neither call site can actually produce. The redundanttypeof value === 'number'guard ahead ofNumber.isIntegerinisIntegeris removed.README: documents each
namingstrategy's exact segment derivation (drop-group,keep-group,basename), since this PR changeskeep-group's own semantics (only the group's own last path segment is kept, not every container segment above it), and documents the empty-glob and nameless-package behaviour above.Added regression tests throughout, including one per rule for a nested object with no name of its own resolving, via the
relativeDirfallback, to the same graph entry as the real top-level manifest (the newrelativeDirconfirmation had started masking this case).A further pair of otherwise-unreachable
assertIsErrorcall-site contexts (inreadDeclaredManifestandvalidateRankRulePatterns) are extracted into their own named functions inworkspace-errors.ts(jsonParseContext,regExpConstructorContext) and tested directly, since neither call site'sassertIsErrorthrow is ever reachable through a realJSON.parseorRegExpfailure.pnpm lint,pnpm typecheck,pnpm testandpnpm buildall pass; CI is green.pnpm test:mutation(stryker.config.ts, threshold 100%) passes clean.