fix(hooks): never format installed plugin and marketplace clones (#2667)
The Stop hook formats every JS/TS file edited during a response, grouped by the project root each file happens to sit in. That includes trees under .claude/plugins, which are third-party checkouts we only read. Formatting them writes to code the user does not own. It also does real damage when a repo's committed code has drifted from its own formatter config: the rewrite is not a no-op but a wholesale reformat, so an unrelated bugfix ends up carrying hundreds of untouched lines. I hit this contributing to this repo — a 162-line fix arrived as a 478-line diff, most of it reformatted code the change never went near. Skips both the user-level install root and a project-local one, mirroring the lookup in scripts/harness-audit.js. Paths are resolved before the prefix comparison, and a sibling such as .claude/plugins-backup does not match. The user own .claude config outside plugins is still formatted. Adds 7 tests for the predicate, plus an end-to-end check that a clone file listed in the accumulator is left byte-identical. Suite 16 to 23. Co-authored-by: haelyra <49814733+haelyra@users.noreply.github.com>
This commit is contained in:
parent
a8c6da485d
commit
7a5757e6c0
|
|
@ -37,6 +37,29 @@ function parseAccumulator(raw) {
|
||||||
return [...new Set(raw.split('\n').map(l => l.trim()).filter(Boolean))];
|
return [...new Set(raw.split('\n').map(l => l.trim()).filter(Boolean))];
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/**
|
||||||
|
* Is this file part of an installed plugin or marketplace clone?
|
||||||
|
*
|
||||||
|
* Those trees are third-party checkouts we merely read. Formatting them writes
|
||||||
|
* to code the user does not own, and when a repo's committed code has drifted
|
||||||
|
* from its own formatter config the rewrite is large: an unrelated bugfix ends
|
||||||
|
* up carrying hundreds of reformatted lines it never touched, which is enough
|
||||||
|
* to sink the contribution it was meant to support.
|
||||||
|
*
|
||||||
|
* Checks both a project-local install root and the user-level one, mirroring
|
||||||
|
* the lookup in scripts/harness-audit.js.
|
||||||
|
*/
|
||||||
|
function isPluginClonePath(filePath, cwd = process.cwd(), homeDir = os.homedir()) {
|
||||||
|
const resolved = path.resolve(filePath);
|
||||||
|
const roots = [path.join(cwd, '.claude', 'plugins')];
|
||||||
|
if (homeDir) roots.push(path.join(homeDir, '.claude', 'plugins'));
|
||||||
|
|
||||||
|
return roots.some(root => {
|
||||||
|
const rel = path.relative(root, resolved);
|
||||||
|
return rel !== '' && !rel.startsWith('..') && !path.isAbsolute(rel);
|
||||||
|
});
|
||||||
|
}
|
||||||
|
|
||||||
function getAccumFile() {
|
function getAccumFile() {
|
||||||
const raw =
|
const raw =
|
||||||
process.env.CLAUDE_SESSION_ID ||
|
process.env.CLAUDE_SESSION_ID ||
|
||||||
|
|
@ -151,6 +174,7 @@ function main() {
|
||||||
const byProjectRoot = new Map();
|
const byProjectRoot = new Map();
|
||||||
for (const filePath of files) {
|
for (const filePath of files) {
|
||||||
if (!/\.(ts|tsx|js|jsx)$/.test(filePath)) continue;
|
if (!/\.(ts|tsx|js|jsx)$/.test(filePath)) continue;
|
||||||
|
if (isPluginClonePath(filePath)) continue;
|
||||||
const resolved = path.resolve(filePath);
|
const resolved = path.resolve(filePath);
|
||||||
if (!fs.existsSync(resolved)) continue;
|
if (!fs.existsSync(resolved)) continue;
|
||||||
const root = findProjectRoot(path.dirname(resolved));
|
const root = findProjectRoot(path.dirname(resolved));
|
||||||
|
|
@ -161,6 +185,7 @@ function main() {
|
||||||
const byTsConfigDir = new Map();
|
const byTsConfigDir = new Map();
|
||||||
for (const filePath of files) {
|
for (const filePath of files) {
|
||||||
if (!/\.(ts|tsx)$/.test(filePath)) continue;
|
if (!/\.(ts|tsx)$/.test(filePath)) continue;
|
||||||
|
if (isPluginClonePath(filePath)) continue;
|
||||||
const resolved = path.resolve(filePath);
|
const resolved = path.resolve(filePath);
|
||||||
if (!fs.existsSync(resolved)) continue;
|
if (!fs.existsSync(resolved)) continue;
|
||||||
const tsDir = findTsConfigDir(resolved);
|
const tsDir = findTsConfigDir(resolved);
|
||||||
|
|
@ -223,4 +248,4 @@ if (require.main === module) {
|
||||||
});
|
});
|
||||||
}
|
}
|
||||||
|
|
||||||
module.exports = { run, parseAccumulator };
|
module.exports = { run, parseAccumulator, isPluginClonePath };
|
||||||
|
|
|
||||||
|
|
@ -13,7 +13,7 @@ const os = require('os');
|
||||||
const path = require('path');
|
const path = require('path');
|
||||||
|
|
||||||
const accumulator = require('../../scripts/hooks/post-edit-accumulator');
|
const accumulator = require('../../scripts/hooks/post-edit-accumulator');
|
||||||
const { parseAccumulator } = require('../../scripts/hooks/stop-format-typecheck');
|
const { parseAccumulator, isPluginClonePath } = require('../../scripts/hooks/stop-format-typecheck');
|
||||||
|
|
||||||
function test(name, fn) {
|
function test(name, fn) {
|
||||||
try {
|
try {
|
||||||
|
|
@ -233,6 +233,46 @@ if (test('stop hook passes stdin through unchanged', () => {
|
||||||
assert.strictEqual(result.toString(), input);
|
assert.strictEqual(result.toString(), input);
|
||||||
})) passed++; else failed++;
|
})) passed++; else failed++;
|
||||||
|
|
||||||
|
// --- Plugin and marketplace clones are read, not owned: never format them ---
|
||||||
|
|
||||||
|
const FAKE_HOME = path.join(path.sep, 'home', 'someone');
|
||||||
|
const FAKE_CWD = path.join(path.sep, 'work', 'project');
|
||||||
|
|
||||||
|
if (test('skips a file inside the user-level plugin install root', () => {
|
||||||
|
const p = path.join(FAKE_HOME, '.claude', 'plugins', 'cache', 'some-plugin', 'scripts', 'tool.js');
|
||||||
|
assert.strictEqual(isPluginClonePath(p, FAKE_CWD, FAKE_HOME), true);
|
||||||
|
})) passed++; else failed++;
|
||||||
|
|
||||||
|
if (test('skips a file inside a marketplace clone', () => {
|
||||||
|
const p = path.join(FAKE_HOME, '.claude', 'plugins', 'marketplaces', 'some-market', 'tests', 'a.test.js');
|
||||||
|
assert.strictEqual(isPluginClonePath(p, FAKE_CWD, FAKE_HOME), true);
|
||||||
|
})) passed++; else failed++;
|
||||||
|
|
||||||
|
if (test('skips a file inside a project-local plugin install root', () => {
|
||||||
|
const p = path.join(FAKE_CWD, '.claude', 'plugins', 'local-plugin', 'index.js');
|
||||||
|
assert.strictEqual(isPluginClonePath(p, FAKE_CWD, FAKE_HOME), true);
|
||||||
|
})) passed++; else failed++;
|
||||||
|
|
||||||
|
if (test('still formats ordinary project files', () => {
|
||||||
|
const p = path.join(FAKE_CWD, 'src', 'app.ts');
|
||||||
|
assert.strictEqual(isPluginClonePath(p, FAKE_CWD, FAKE_HOME), false);
|
||||||
|
})) passed++; else failed++;
|
||||||
|
|
||||||
|
if (test('still formats the user own .claude config outside plugins', () => {
|
||||||
|
const p = path.join(FAKE_HOME, '.claude', 'scripts', 'hooks', 'mine.js');
|
||||||
|
assert.strictEqual(isPluginClonePath(p, FAKE_CWD, FAKE_HOME), false);
|
||||||
|
})) passed++; else failed++;
|
||||||
|
|
||||||
|
if (test('does not match a sibling directory sharing the prefix', () => {
|
||||||
|
const p = path.join(FAKE_HOME, '.claude', 'plugins-backup', 'thing.js');
|
||||||
|
assert.strictEqual(isPluginClonePath(p, FAKE_CWD, FAKE_HOME), false);
|
||||||
|
})) passed++; else failed++;
|
||||||
|
|
||||||
|
if (test('resolves traversal before deciding', () => {
|
||||||
|
const p = path.join(FAKE_CWD, 'src', '..', '.claude', 'plugins', 'p', 'x.js');
|
||||||
|
assert.strictEqual(isPluginClonePath(p, FAKE_CWD, FAKE_HOME), true);
|
||||||
|
})) passed++; else failed++;
|
||||||
|
|
||||||
// Restore env
|
// Restore env
|
||||||
if (origSessionId === undefined) {
|
if (origSessionId === undefined) {
|
||||||
delete process.env.CLAUDE_SESSION_ID;
|
delete process.env.CLAUDE_SESSION_ID;
|
||||||
|
|
|
||||||
Loading…
Reference in New Issue