Fix Recipes failing due to pnpm workspace/catalog in skeleton and line number drifts#3552
Fix Recipes failing due to pnpm workspace/catalog in skeleton and line number drifts#3552
Conversation
|
Oxygen deployed a preview of your
Learn more about Hydrogen's GitHub integration. |
![binks-code-reviewer[bot]](https://avatars.githubusercontent.com/in/2788732?s=60&v=4){kind=small}
cookbook/src/lib/validate.ts
Outdated
| execSync(`git checkout -- ${modifiedPkgJsons.split('\n').join(' ')}`, { | ||
| cwd: REPO_ROOT, | ||
| stdio: 'pipe', | ||
| }); |
There was a problem hiding this comment.
Cleanup uses unsafe shell string construction to restore modified package.json files
In finally, restoration runs execSync(git checkout -- ${modifiedPkgJsons.split('\n').join(' ')}), concatenating unescaped filenames into a shell command.
Impact: breaks with spaces/shell-special characters in paths and can leave the repo dirty. It’s also generally unsafe (potential injection in hostile environments). Cleanup failures are particularly harmful because they defeat the guarantee of restoring modified files.
|
🤖 Code Review · #projects-dev-ai for questions ✅ Complete - No issues 📋 History✅ 1 findings → ✅ No issues |
cookbook/src/commands/apply.ts
Outdated
| }; | ||
|
|
||
| function handler(args: ApplyArgs) { | ||
| resolveCatalogProtocol(); |
There was a problem hiding this comment.
blocking: resolveCatalogProtocol() modifies workspace package.json files but apply and regenerate never restore them.
resolveCatalogProtocol() writes resolved versions into ALL workspace package.json files (root, packages/hydrogen, packages/cli, templates/skeleton, etc.). The validate command has a finally block (validate.ts:513-531) that restores these via git checkout, but neither apply nor regenerate has any such cleanup.
After running npx cookbook apply --recipe express, every workspace package.json with catalog: or workspace: references will be modified with inline versions. Same for regenerate — after the loop finishes, the workspace package.json files are left dirty. Only TEMPLATE_PATH gets restored via git checkout -- . and git clean -fd.
Additionally in regenerate, if resolveCatalogProtocol() itself throws, the existing git checkout/git clean lines (94-95) never run, so even the template directory is left dirty.
This is the same root problem as the scattered call sites comment below — the resolution step and cleanup are separated from the operation they support. I think the cleanest fix is to encapsulate the resolve-apply-cleanup lifecycle (see the design suggestion on validate.ts:428).
cookbook/src/lib/validate.ts
Outdated
|
|
||
| // Resolve in all workspace package.json files since npm traverses the workspace | ||
| const workspacePkgPaths: string[] = [path.join(REPO_ROOT, 'package.json')]; | ||
| const workspaceYaml = YAML.parse(workspaceContent); |
There was a problem hiding this comment.
non-blocking: double-parse of pnpm-workspace.yaml.
Line 591 already parses the workspace YAML into workspace, and this line parses the exact same workspaceContent again into workspaceYaml. These are identical objects from the same string.
I think this line should just reuse the already-parsed workspace:
| const workspaceYaml = YAML.parse(workspaceContent); | |
| const workspacePackages: string[] = workspace?.packages ?? []; |
This also removes the confusing dual naming (workspace vs workspaceYaml for the same thing).
cookbook/src/lib/validate.ts
Outdated
| } | ||
|
|
||
| console.log('- 🔄 Resolving catalog: and workspace: protocol references…'); | ||
| resolveCatalogProtocol(); |
There was a problem hiding this comment.
non-blocking: resolveCatalogProtocol() is called in 3 separate locations, requiring each caller to remember to call it before applyRecipe() and clean up after.
The knowledge that catalog/workspace protocols must be resolved before applying a recipe — and restored afterward — is scattered across:
validate.ts:428(insidevalidateRecipe, with cleanup infinally)commands/apply.ts:23(beforeapplyRecipe, no cleanup)commands/regenerate.ts:75(beforeapplyRecipein loop, no cleanup)
This is the same root problem as the blocking comment on apply.ts. Both the missing cleanup and the scattered call sites would be solved by encapsulating this inside applyRecipe() itself, or by introducing a withResolvedCatalog(fn) wrapper that handles resolve-execute-cleanup as a single unit. That way new callers can't forget to resolve or forget to clean up.
Not blocking since the current 3 call sites are correct (apart from the cleanup gap), but this is the design fix I'd recommend for the blocking issue.
cookbook/src/lib/validate.ts
Outdated
| typeof version === 'string' && | ||
| version.startsWith('workspace:') | ||
| ) { | ||
| const workspaceVersion = resolveWorkspaceVersion(name); |
There was a problem hiding this comment.
non-blocking: workspace:^ and workspace:~ protocols are resolved to bare versions, losing the range prefix.
pnpm's workspace: protocol supports range specifiers:
workspace:*should resolve to exact version (e.g.,2026.1.0) — this works correctlyworkspace:^should resolve to^2026.1.0workspace:~should resolve to~2026.1.0
Currently all workspace: variants are resolved to the raw version from resolveWorkspaceVersion(). For the cookbook validation use case this might be fine since we just need npm install to succeed, but if any recipe package.json uses workspace:^, the resolved version would be more restrictive than intended.
cookbook/src/lib/validate.ts
Outdated
|
|
||
| for (const [name, version] of Object.entries(deps)) { | ||
| if (version === 'catalog:' || version === 'catalog:default') { | ||
| if (catalog[name]) { |
There was a problem hiding this comment.
non-blocking: missing catalog: entries are silently swallowed.
When a dependency references catalog:some-key but that key doesn't exist in pnpm-workspace.yaml's catalog, the if (catalog[name]) guard silently falls through without modifying the dependency. It keeps its unresolvable catalog:some-key value, and the subsequent npm install will fail with a confusing error rather than a clear message about the missing catalog entry.
I think a console.warn (or even an error) here would save debugging time — something like "Warning: catalog key 'some-key' not found in pnpm-workspace.yaml".
cookbook/src/lib/validate.ts
Outdated
| let _workspaceVersions: Map<string, string> | null = null; | ||
|
|
||
| /** @internal Reset cached workspace versions (for testing) */ | ||
| export function _resetWorkspaceVersionsCache(): void { |
There was a problem hiding this comment.
nit: exporting _resetWorkspaceVersionsCache() from production code for testing.
The _ prefix convention signals this is internal, but it's still part of the public API surface. If this caching pattern grows, dependency injection (passing the cache or a reader function) would be more testable without exporting internals. Fine for now though.
cookbook/src/lib/validate.ts
Outdated
| * by looking up versions from pnpm-workspace.yaml catalog. | ||
| * This is needed because `npm install` doesn't understand the `catalog:` protocol. | ||
| */ | ||
| export function resolveCatalogProtocol(): void { |
There was a problem hiding this comment.
non-blocking: resolveCatalogProtocol() lives in validate.ts but isn't validation.
This function resolves pnpm protocols so that npm can install — it's workspace setup, not validation. Having it in validate.ts means apply.ts and regenerate.ts both import from the validation module for something that has nothing to do with validation. If the cookbook grows, this would be better in its own module (e.g., workspace.ts) or co-located with apply.ts. Not urgent, but worth noting for future clarity.
| status.modifiedFiles.filter( | ||
| (f) => !['package.json', 'package-lock.json'].includes(f), | ||
| (f) => | ||
| !['package.json', 'package-lock.json'].includes(path.basename(f)), |
There was a problem hiding this comment.
praise: nice catch on the path.basename fix.
The old code compared full paths like templates/skeleton/package.json against just package.json, which would never match for subdirectory files. path.basename(f) is the correct fix.
| function installDependencies(): Command { | ||
| return { | ||
| command: 'npm install', | ||
| command: 'npm install --no-workspaces --ignore-scripts', |
There was a problem hiding this comment.
praise: npm install --no-workspaces --ignore-scripts flags are a good addition.
--no-workspaces prevents npm from trying to process pnpm workspace configuration it doesn't understand, and --ignore-scripts avoids running lifecycle scripts in a validation-only context. Both are appropriate for this use case.
| @@ -0,0 +1,201 @@ | |||
| # Fix Cookbook Recipe Validation Failures | |||
There was a problem hiding this comment.
praise: the Claude command for fixing recipe validation is well-structured.
The 5-phase approach (Validate, Fix Patches, Fix Other Issues, Re-validate, Handle Multiple Recipes) with clear failure categories (A-E) is a thoughtful guide for automated recipe fixing.
2 participants
WHY are these changes introduced?
The Hydrogen monorepo recently adopted pnpm's
catalog:andworkspace:protocols inpnpm-workspace.yamlfor managing dependency versions across packages. The cookbook recipe validation system usesnpm installto verify that recipes apply cleanly to the skeleton template, but npm doesn't understand these pnpm-specific protocols, causing all recipe validations to fail. Additionally, many recipe patches had drifted due to recent skeleton template changes (line number shifts, content changes. Potentially from prettier fix.).WHAT is this pull request doing?
1. Adds
catalog:andworkspace:protocol resolution to the cookbook validation systemresolveCatalogProtocol()incookbook/src/lib/validate.tsthat readspnpm-workspace.yamland replacescatalog:/catalog:defaultreferences with actual versions from the catalog, andworkspace:*references with the package's real version from the monorepopackage.jsonfiles before runningnpm install, since npm traverses the workspace treepackage.jsonfiles in thefinallyblock after validation completesnpm installto use--no-workspaces --ignore-scriptsflags to avoid workspace-related issues during validationapply.tsto usepath.basename(f)sopackage.jsonfiles in subdirectories are also properly ignored2. Calls
resolveCatalogProtocol()in theapplyandregeneratecommands so patches can be regenerated correctly against the resolved skeleton.3. Fixes stale patches across multiple recipes:
b2b,bundles,combined-listings,express,legacy-customer-account-flow,markets,metaobjects,multipass,partytown,subscriptions,third-party-queries-cachingtsconfig.jsonpatch for the express recipe4. Adds a Claude Code command (
.claude/commands/fix-cookbook-recipe.md) for automating recipe patch fixes when skeleton template changes cause validation drift.5. Adds tests for the new
resolveCatalogProtocolfunctionality covering catalog resolution, workspace resolution,catalog:defaultsyntax, and edge cases.HOW to test your changes?
cookbook/directory, run validation for all recipes:Post-merge steps
None required.
Checklist