From 969456c46d6617b990b93858064a34545e02f2ad Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Sat, 19 Sep 2026 10:55:01 -0400 Subject: [PATCH] fix(#4759): hooks marker warning states the will-not-load claim conditionally, like the plugin path (#4875) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * test(#4759): failing-first — preserved-foreign hooks warning must not claim may-not-load for a commonjs package.json * fix(#4759): hooks marker warning states the will-not-load claim conditionally, like the plugin path * test(#4759): review fold-ins — assert installer output on stdout+stderr (warn is stderr), register cleanup before the spawn, add the type-less foreign case * chore(#4759): changeset fragment * chore(#4759): backfill changeset PR number (4875) --------- Co-authored-by: sim --- .changeset/sturdy-seals-click.md | 5 ++ bin/install.js | 9 ++- tests/hooks-commonjs-marker.test.cjs | 106 +++++++++++++++++++++++++++ 3 files changed, 119 insertions(+), 1 deletion(-) create mode 100644 .changeset/sturdy-seals-click.md diff --git a/.changeset/sturdy-seals-click.md b/.changeset/sturdy-seals-click.md new file mode 100644 index 000000000..cf6c47f02 --- /dev/null +++ b/.changeset/sturdy-seals-click.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 4875 +--- +**Installer stops warning that hooks may not load when they already load as CommonJS** — installing over an existing hooks/package.json that declares "type": "commonjs" (any hand-written or formatter-touched file) printed "GSD hooks may not resolve as CommonJS"; the warning now states will-not-load only for the case that is true, a declared "type": "module", matching the plugin-path wording. (#4759) diff --git a/bin/install.js b/bin/install.js index 8295b6392..54a1e8f3d 100755 --- a/bin/install.js +++ b/bin/install.js @@ -12122,7 +12122,14 @@ function install(isGlobal, runtime = DEFAULT_RUNTIME, options = {}) { console.log(` ${green}✓${reset} Wrote ${sharedHooksDirName}/package.json (CommonJS mode)`); break; case 'preserved-foreign': - console.warn(` ${yellow}⚠${reset} Left existing ${sharedHooksDirName}/package.json untouched (not GSD's marker) — GSD hooks may not resolve as CommonJS`); + // #4759: the foreign file usually DOES declare "type": "commonjs" — + // any hand-written or formatter-touched package.json does — and Node + // then loads the staged .js hooks as CommonJS, so the old + // unconditional "may not resolve" claim was usually false. The + // sibling plugin path (src/install-engine.cts) words this same + // outcome conditionally; match it and keep will-not-load conditional + // on "type": "module", the only case where it is true. + console.warn(` ${yellow}⚠${reset} Left existing ${sharedHooksDirName}/package.json untouched (not GSD's marker). If it declares "type": "module", the staged hooks will not load.`); break; case 'failed': // Best-effort: a read-only or full config dir must not abort the diff --git a/tests/hooks-commonjs-marker.test.cjs b/tests/hooks-commonjs-marker.test.cjs index bae618a10..364afb80f 100644 --- a/tests/hooks-commonjs-marker.test.cjs +++ b/tests/hooks-commonjs-marker.test.cjs @@ -145,4 +145,110 @@ describe('#2717 CommonJS marker for staged .js hooks', () => { assert.ok(!ensureCommonJsMarker(dirD), 'does not overwrite a user-authored package.json'); assert.strictEqual(fs.readFileSync(path.join(dirD, 'package.json'), 'utf8'), userContent); }); + + // #4759: the shared hooks path's `preserved-foreign` report used to make an + // unconditional module-resolution claim ("GSD hooks may not resolve as + // CommonJS") that is FALSE for the ordinary case — any foreign file that does + // not declare {"type":"module"} leaves the staged .js hooks loading as + // CommonJS (Node's default). The sibling plugin path + // (src/install-engine.cts) words the same outcome conditionally; these tests + // pin the hooks path to the same shape: name the ownership fact, and make + // will-not-load conditional on "type": "module" — the only case where it is + // true. + test('#4759: preserved-foreign hooks marker warning must not claim the hooks may not load when the file declares commonjs', (t) => { + const foreignCommonJs = '{\n "type": "commonjs"\n}\n'; + const root = createTempDir('gsd-4759-commonjs-'); + // Register cleanup BEFORE the spawn: runMinimalInstall deliberately never + // removes a caller-provided root, on the failure path either, so a spawn + // failure must not leak the temp tree. + t.after(() => cleanup(root)); + // Plant the foreign file BEFORE the install, at the config dir the global + // install will resolve (RUNTIME_META.claude.globalSuffix = '.claude'). + const hooksDir = path.join(root, '.claude', 'hooks'); + fs.mkdirSync(hooksDir, { recursive: true }); + fs.writeFileSync(path.join(hooksDir, 'package.json'), foreignCommonJs); + + // The marker notice is a console.warn — stderr. Assert on the combined + // output so a future stream change cannot silently vacate the guards + // (precedent: tests/install.test.cjs merges stdout+stderr for installer + // output assertions). + const { configDir, stdout, stderr } = runMinimalInstall({ runtime: 'claude', scope: 'global', root }); + const output = stdout + stderr; + assert.strictEqual(configDir, path.join(root, '.claude')); + + // Ownership behavior is NOT the bug — the foreign file stays byte-identical. + assert.strictEqual( + fs.readFileSync(path.join(configDir, 'hooks', 'package.json'), 'utf8'), + foreignCommonJs, + 'the foreign package.json must be left untouched', + ); + + // The false claim must be gone. + assert.ok( + !output.includes('may not resolve as CommonJS'), + `a foreign package.json declaring "type": "commonjs" must not be told the hooks may not load; output:\n${output}`, + ); + // The preserved-foreign notice stays, now with the conditional wording. + assert.ok( + /Left existing hooks\/package\.json untouched \(not GSD's marker\)\. If it declares "type": "module", the staged hooks will not load\./.test(output), + `expected the conditional preserved-foreign wording; output:\n${output}`, + ); + }); + + test('#4759: a module-typed foreign hooks package.json gets the conditional will-not-load warning', (t) => { + const foreignEsm = '{\n "type": "module"\n}\n'; + const root = createTempDir('gsd-4759-esm-'); + t.after(() => cleanup(root)); + const hooksDir = path.join(root, '.claude', 'hooks'); + fs.mkdirSync(hooksDir, { recursive: true }); + fs.writeFileSync(path.join(hooksDir, 'package.json'), foreignEsm); + + const { configDir, stdout, stderr } = runMinimalInstall({ runtime: 'claude', scope: 'global', root }); + const output = stdout + stderr; + + assert.strictEqual( + fs.readFileSync(path.join(configDir, 'hooks', 'package.json'), 'utf8'), + foreignEsm, + 'the foreign package.json must be left untouched', + ); + // Still no UNCONDITIONAL may-claim — the will-not-load statement is carried + // by the conditional sentence, which is true here and only here. + assert.ok( + !output.includes('may not resolve as CommonJS'), + `the unconditional may-not-load claim must not come back; output:\n${output}`, + ); + assert.ok( + /If it declares "type": "module", the staged hooks will not load\./.test(output), + `a "type": "module" foreign file must still get the conditional will-not-load warning; output:\n${output}`, + ); + }); + + test('#4759: a foreign hooks package.json with no type field gets the conditional wording, not the may-claim', (t) => { + // No "type" field: Node defaults .js to CommonJS, so the hooks load — the + // old unconditional claim was false here too, and this is arguably the most + // common real file (any hand-authored package.json). + const foreignNoType = '{\n "name": "user-hooks"\n}\n'; + const root = createTempDir('gsd-4759-notype-'); + t.after(() => cleanup(root)); + const hooksDir = path.join(root, '.claude', 'hooks'); + fs.mkdirSync(hooksDir, { recursive: true }); + fs.writeFileSync(path.join(hooksDir, 'package.json'), foreignNoType); + + const { configDir, stdout, stderr } = runMinimalInstall({ runtime: 'claude', scope: 'global', root }); + const output = stdout + stderr; + + assert.strictEqual( + fs.readFileSync(path.join(configDir, 'hooks', 'package.json'), 'utf8'), + foreignNoType, + 'the foreign package.json must be left untouched', + ); + assert.ok( + !output.includes('may not resolve as CommonJS'), + `a type-less foreign package.json must not be told the hooks may not load; output:\n${output}`, + ); + assert.ok( + /If it declares "type": "module", the staged hooks will not load\./.test(output), + `expected the conditional preserved-foreign wording; output:\n${output}`, + ); + }); });