diff --git a/scripts/lib/install/apply.js b/scripts/lib/install/apply.js index 6c4736858..6d5bb7171 100644 --- a/scripts/lib/install/apply.js +++ b/scripts/lib/install/apply.js @@ -5,6 +5,30 @@ const path = require('path'); const { writeInstallState } = require('../install-state'); const { filterMcpConfig, parseDisabledMcpServers } = require('../mcp-config'); +const { buildInstallIndex, isNamespacedSource, rewriteRelativeLinks } = require('./link-rewrite'); + +function isMarkdownPath(filePath) { + return /\.(md|mdx|markdown)$/i.test(String(filePath || '')); +} + +// Map every copy-file operation to { sourceRel, destRel } so relative links in +// namespaced markdown can be rewritten to the file's actual installed location +// (issue #2340). Returns null when the plan lacks the data needed to do so. +function buildLinkIndexForPlan(plan) { + if (!plan || !plan.targetRoot || !Array.isArray(plan.operations)) { + return null; + } + const mappings = []; + for (const operation of plan.operations) { + if (operation.kind === 'copy-file' && operation.sourceRelativePath) { + mappings.push({ + sourceRel: operation.sourceRelativePath, + destRel: path.relative(plan.targetRoot, operation.destinationPath), + }); + } + } + return buildInstallIndex(mappings); +} function readJsonObject(filePath, label) { let parsed; @@ -118,6 +142,7 @@ function buildResolvedClaudeHooks(plan) { function applyInstallPlan(plan) { const resolvedClaudeHooksPlan = buildResolvedClaudeHooks(plan); const disabledServers = parseDisabledMcpServers(process.env.ECC_DISABLED_MCPS); + const linkIndex = buildLinkIndexForPlan(plan); for (const operation of plan.operations) { fs.mkdirSync(path.dirname(operation.destinationPath), { recursive: true }); @@ -149,6 +174,25 @@ function applyInstallPlan(plan) { continue; } + // Namespaced markdown (e.g. skills/ -> skills/ecc/) needs its + // relative cross-directory links rewritten so they resolve after install + // (issue #2340). Files whose install path is unchanged (no namespace + // injected) and all non-markdown files stay on the byte-for-byte copy path. + if ( + linkIndex + && operation.kind === 'copy-file' + && operation.sourceRelativePath + && isMarkdownPath(operation.destinationPath) + && isNamespacedSource(operation.sourceRelativePath, linkIndex) + ) { + const rewritten = rewriteRelativeLinks( + fs.readFileSync(operation.sourcePath, 'utf8'), + { sourceRel: operation.sourceRelativePath, index: linkIndex } + ); + fs.writeFileSync(operation.destinationPath, rewritten, 'utf8'); + continue; + } + fs.copyFileSync(operation.sourcePath, operation.destinationPath); } diff --git a/scripts/lib/install/link-rewrite.js b/scripts/lib/install/link-rewrite.js new file mode 100644 index 000000000..53b1beb12 --- /dev/null +++ b/scripts/lib/install/link-rewrite.js @@ -0,0 +1,179 @@ +'use strict'; + +const path = require('path'); + +const posix = path.posix; + +// Matches inline markdown links and images: `](target)` / `](target "title")`. +// We deliberately scope to the inline form because that is what skill/rule docs +// use for cross-directory references. Reference-style and autolinks are left +// untouched (they are rare in these files and carry higher false-positive risk). +const INLINE_LINK_PATTERN = /(!?\]\()([^()\s]+)(\s+"[^"]*")?(\))/g; + +function toPosix(relativePath) { + return String(relativePath || '').replace(/\\/g, '/').replace(/^\.\//, ''); +} + +function stripTrailingSlash(value) { + return value.length > 1 ? value.replace(/\/+$/, '') : value; +} + +// Build file + directory lookup maps from the plan's own file placements. +// `fileMappings` is a list of { sourceRel, destRel } where both are paths +// relative to the repo root and the install root respectively. The directory +// map is derived by walking shared ancestors of each source/dest pair, which is +// exact for prefix-insertion namespacing (e.g. `skills/x` -> `skills/ecc/x`): +// the path suffix below the inserted segment is preserved, so ancestor `k` +// of the source maps to the dest with the matching number of trailing +// segments removed. +function buildInstallIndex(fileMappings) { + const byFile = new Map(); + const byDir = new Map(); + + for (const mapping of fileMappings || []) { + const sourceRel = toPosix(mapping.sourceRel); + const destRel = toPosix(mapping.destRel); + if (!sourceRel || !destRel) { + continue; + } + + byFile.set(sourceRel, destRel); + + const sourceParts = sourceRel.split('/'); + const destParts = destRel.split('/'); + // Map every source ancestor directory to its installed counterpart by + // removing the same count of trailing segments from the dest path. + for (let depth = 1; depth < sourceParts.length; depth += 1) { + const trailing = sourceParts.length - depth; + const destDepth = destParts.length - trailing; + if (destDepth < 1) { + continue; + } + const sourceDir = sourceParts.slice(0, depth).join('/'); + const destDir = destParts.slice(0, destDepth).join('/'); + // Only record real prefix-insertion mappings (suffix preserved). If a + // directory resolves to itself (no namespace change) we skip it so the + // rewriter leaves those links alone. + if (sourceDir !== destDir) { + byDir.set(sourceDir, destDir); + } + } + } + + return { byFile, byDir }; +} + +function isExternalOrAnchor(target) { + return ( + target === '' + || target.startsWith('#') + || target.startsWith('/') + || target.startsWith('mailto:') + || /^[a-zA-Z][a-zA-Z0-9+.-]*:/.test(target) // has a URL scheme (http:, https:, file:, ...) + ); +} + +// Resolve `target` (a relative link from `sourceDir`) to its repo-relative +// path, then return the install-relative path it should point to, or null when +// the target is not installed by this plan (leave such links untouched). +function resolveInstalledTarget(target, sourceDir, index) { + const hadTrailingSlash = target.endsWith('/'); + const resolved = stripTrailingSlash(toPosix(posix.normalize(posix.join(sourceDir, target)))); + + // Escapes the repo root (starts with `..`) -> not something we placed. + if (resolved === '' || resolved === '.' || resolved.startsWith('..')) { + return null; + } + + if (!hadTrailingSlash && index.byFile.has(resolved)) { + return { installed: index.byFile.get(resolved), trailingSlash: false }; + } + if (index.byDir.has(resolved)) { + return { installed: index.byDir.get(resolved), trailingSlash: hadTrailingSlash }; + } + return null; +} + +// True when the plan installs `sourceRel` at a different relative path than the +// source (i.e. a namespace segment was injected, e.g. skills/x -> skills/ecc/x). +// Callers use this to keep non-namespaced files on the byte-for-byte copy path. +function isNamespacedSource(sourceRel, index) { + const normalizedSource = toPosix(sourceRel); + const installedSource = index && index.byFile.get(normalizedSource); + return Boolean(installedSource) && installedSource !== normalizedSource; +} + +// Rewrite relative links in a single namespaced markdown file so they resolve +// to the file's installed location. Returns the content unchanged when the +// file itself was not namespaced or when no link needs adjustment. Pure: no IO. +function rewriteRelativeLinks(content, options) { + const { sourceRel, index } = options || {}; + const normalizedSource = toPosix(sourceRel); + const installedSource = index && index.byFile.get(normalizedSource); + + // Only rewrite when the file's own install path gained/changed a namespace + // segment. If it lands at the same relative path, every link recomputes to + // itself, so there is nothing to do. + if (!installedSource || installedSource === normalizedSource) { + return content; + } + + const installedSourceDir = posix.dirname(installedSource); + const sourceDir = posix.dirname(normalizedSource); + const lines = String(content).split('\n'); + let inFence = false; + + for (let i = 0; i < lines.length; i += 1) { + const fenceToggle = /^\s*(```|~~~)/.test(lines[i]); + if (fenceToggle) { + inFence = !inFence; + continue; + } + if (inFence) { + continue; // never rewrite inside fenced code blocks + } + + lines[i] = lines[i].replace( + INLINE_LINK_PATTERN, + (match, open, target, title, close) => { + // Preserve any `#fragment` so anchors survive the rewrite. + const hashIdx = target.indexOf('#'); + const pathPart = hashIdx === -1 ? target : target.slice(0, hashIdx); + const fragment = hashIdx === -1 ? '' : target.slice(hashIdx); + + if (isExternalOrAnchor(pathPart)) { + return match; + } + + const resolution = resolveInstalledTarget(pathPart, sourceDir, index); + if (!resolution) { + return match; + } + + let rewritten = posix.relative(installedSourceDir, resolution.installed); + if (rewritten === '') { + rewritten = '.'; + } + if (resolution.trailingSlash && !rewritten.endsWith('/')) { + rewritten += '/'; + } + // If the recomputed link points to the same place as the original + // (e.g. an intra-namespace `./sibling.md` whose endpoints both shift by + // the same prefix), keep the original text verbatim — including any + // leading `./` — so the rewrite stays a strict no-op where it must. + if (posix.normalize(rewritten) === posix.normalize(pathPart)) { + return match; + } + return `${open}${rewritten}${fragment}${title || ''}${close}`; + } + ); + } + + return lines.join('\n'); +} + +module.exports = { + buildInstallIndex, + isNamespacedSource, + rewriteRelativeLinks, +}; diff --git a/tests/lib/install-link-rewrite.test.js b/tests/lib/install-link-rewrite.test.js new file mode 100644 index 000000000..b4a75115d --- /dev/null +++ b/tests/lib/install-link-rewrite.test.js @@ -0,0 +1,231 @@ +/** + * Tests for scripts/lib/install/link-rewrite.js — rewriting relative links in + * namespaced markdown so they resolve after install (issue #2340). + */ + +'use strict'; + +const assert = require('assert'); +const path = require('path'); + +const { + buildInstallIndex, + isNamespacedSource, + rewriteRelativeLinks, +} = require('../../scripts/lib/install/link-rewrite'); +const { createManifestInstallPlan } = require('../../scripts/lib/install-executor'); + +const REPO_ROOT = path.resolve(__dirname, '..', '..'); + +// A claude-style namespace placement: skills/ -> skills/ecc/ and +// rules/ -> rules/ecc/. Mirrors what the real adapter emits. +function claudeNamespaceMappings() { + return [ + { sourceRel: 'skills/react-patterns/SKILL.md', destRel: 'skills/ecc/react-patterns/SKILL.md' }, + { sourceRel: 'skills/react-patterns/other.md', destRel: 'skills/ecc/react-patterns/other.md' }, + { sourceRel: 'skills/react-patterns/sub/NOTE.md', destRel: 'skills/ecc/react-patterns/sub/NOTE.md' }, + { sourceRel: 'rules/react/hooks.md', destRel: 'rules/ecc/react/hooks.md' }, + { sourceRel: 'rules/react/testing.md', destRel: 'rules/ecc/react/testing.md' }, + { sourceRel: 'rules/react/coding-style.md', destRel: 'rules/ecc/react/coding-style.md' }, + ]; +} + +// An identity placement (non-namespacing adapter): source == dest. +function identityMappings() { + return [ + { sourceRel: 'skills/react-patterns/SKILL.md', destRel: 'skills/react-patterns/SKILL.md' }, + { sourceRel: 'rules/react/hooks.md', destRel: 'rules/react/hooks.md' }, + ]; +} + +function test(name, fn) { + try { + fn(); + console.log(` PASS ${name}`); + return true; + } catch (error) { + console.log(` FAIL ${name}`); + console.log(` Error: ${error.message}`); + // Preserve the full stack (source line + assertion diff) for diagnosis. + console.error(error.stack || error); + return false; + } +} + +function runTests() { + console.log('Running install link-rewrite tests...\n'); + let passed = 0; + let failed = 0; + + const index = buildInstallIndex(claudeNamespaceMappings()); + + // Parametrize the canonical file-link case over all three affected skills so + // a regression in any one is caught (not just the first). + for (const skill of ['react-patterns', 'react-performance', 'react-testing']) { + if (test(`rewrites ../../rules file link for ${skill}`, () => { + const idx = buildInstallIndex([ + { sourceRel: `skills/${skill}/SKILL.md`, destRel: `skills/ecc/${skill}/SKILL.md` }, + { sourceRel: 'rules/react/hooks.md', destRel: 'rules/ecc/react/hooks.md' }, + ]); + const before = 'See [rules](../../rules/react/hooks.md) for details.'; + const after = rewriteRelativeLinks(before, { sourceRel: `skills/${skill}/SKILL.md`, index: idx }); + assert.notStrictEqual(after, before, 'rewrite must change the broken link (not vacuous)'); + assert.ok( + after.includes('](../../../rules/ecc/react/hooks.md)'), + `expected corrected link, got: ${after}` + ); + assert.ok(!after.includes('](../../rules/'), 'broken depth must be gone'); + })) passed++; else failed++; + } + + if (test('rewrites a directory link and preserves the trailing slash', () => { + const before = '- Rules: [rules/react/](../../rules/react/)'; + const after = rewriteRelativeLinks(before, { sourceRel: 'skills/react-patterns/SKILL.md', index }); + assert.notStrictEqual(after, before); + assert.ok(after.includes('](../../../rules/ecc/react/)'), `got: ${after}`); + })) passed++; else failed++; + + if (test('leaves an intra-skill sibling link unchanged', () => { + const before = 'Look at [other](./other.md) nearby.'; + const after = rewriteRelativeLinks(before, { sourceRel: 'skills/react-patterns/SKILL.md', index }); + assert.strictEqual(after, before, 'same-prefix relative path must be preserved'); + })) passed++; else failed++; + + if (test('leaves links to non-installed targets unchanged', () => { + const before = 'See [pkg](../../package.json) at the root.'; + const after = rewriteRelativeLinks(before, { sourceRel: 'skills/react-patterns/SKILL.md', index }); + assert.strictEqual(after, before, 'never invent a path to a file the plan does not install'); + })) passed++; else failed++; + + if (test('leaves external urls, absolute paths, and anchors unchanged', () => { + const before = [ + '[ext](https://example.com/rules/react/hooks.md)', + '[abs](/rules/react/hooks.md)', + '[mail](mailto:x@example.com)', + '[anchor](#a-section)', + ].join('\n'); + const after = rewriteRelativeLinks(before, { sourceRel: 'skills/react-patterns/SKILL.md', index }); + assert.strictEqual(after, before); + })) passed++; else failed++; + + if (test('preserves a #fragment on a rewritten link', () => { + const before = '[hooks](../../rules/react/hooks.md#use-effect)'; + const after = rewriteRelativeLinks(before, { sourceRel: 'skills/react-patterns/SKILL.md', index }); + assert.ok(after.includes('](../../../rules/ecc/react/hooks.md#use-effect)'), `got: ${after}`); + })) passed++; else failed++; + + if (test('does not rewrite links inside fenced code blocks', () => { + const before = [ + '```md', + '[code](../../rules/react/hooks.md)', + '```', + '[prose](../../rules/react/hooks.md)', + ].join('\n'); + const after = rewriteRelativeLinks(before, { sourceRel: 'skills/react-patterns/SKILL.md', index }); + assert.ok(after.includes('[code](../../rules/react/hooks.md)'), 'code-fence link must be untouched'); + assert.ok(after.includes('[prose](../../../rules/ecc/react/hooks.md)'), 'prose link must be rewritten'); + })) passed++; else failed++; + + if (test('computes depth from path math for a nested skill file', () => { + // skills/react-patterns/sub/NOTE.md -> skills/ecc/react-patterns/sub/NOTE.md + // Source link is ../../../rules/react/hooks.md (3 up from sub/). + const before = '[r](../../../rules/react/hooks.md)'; + const after = rewriteRelativeLinks(before, { sourceRel: 'skills/react-patterns/sub/NOTE.md', index }); + assert.notStrictEqual(after, before, 'nested depth must be recomputed, not hardcoded'); + assert.ok(after.includes('](../../../../rules/ecc/react/hooks.md)'), `got: ${after}`); + })) passed++; else failed++; + + if (test('is a no-op for a non-namespacing (identity) placement', () => { + const idx = buildInstallIndex(identityMappings()); + const before = '[r](../../rules/react/hooks.md)'; + const after = rewriteRelativeLinks(before, { sourceRel: 'skills/react-patterns/SKILL.md', index: idx }); + assert.strictEqual(after, before, 'identity placement must not touch any link'); + })) passed++; else failed++; + + if (test('is a no-op when the file itself is not in the plan', () => { + const before = '[r](../../rules/react/hooks.md)'; + const after = rewriteRelativeLinks(before, { sourceRel: 'skills/not-installed/SKILL.md', index }); + assert.strictEqual(after, before); + })) passed++; else failed++; + + // Guards the apply-layer gate: only namespaced files leave the byte-copy + // path, so non-namespaced markdown is still copied verbatim. + if (test('isNamespacedSource flags only files whose install path changed', () => { + assert.strictEqual( + isNamespacedSource('skills/react-patterns/SKILL.md', index), true, + 'a namespaced skill file must be flagged' + ); + const identity = buildInstallIndex(identityMappings()); + assert.strictEqual( + isNamespacedSource('skills/react-patterns/SKILL.md', identity), false, + 'an identity-mapped file must stay on the byte-copy path' + ); + assert.strictEqual( + isNamespacedSource('skills/not-in-plan/SKILL.md', index), false, + 'a file the plan does not install is not namespaced' + ); + })) passed++; else failed++; + + // Integration: real repo content + real claude plan. Every rewritten link in + // the three React skills must resolve to a destination the SAME plan installs. + if (test('real React skills: rewritten rules links resolve to installed targets', () => { + const fs = require('fs'); + const plan = createManifestInstallPlan({ + sourceRoot: REPO_ROOT, + homeDir: '/tmp/ecc-link-rewrite-it', + target: 'claude', + moduleIds: ['framework-language', 'rules-core'], + }); + const mappings = plan.operations + .filter(op => op.kind === 'copy-file' && op.sourceRelativePath) + .map(op => ({ + sourceRel: op.sourceRelativePath, + destRel: path.relative(plan.targetRoot, op.destinationPath), + })); + const realIndex = buildInstallIndex(mappings); + const installedDestRels = new Set(mappings.map(m => m.destRel.replace(/\\/g, '/'))); + + const extractLinks = text => { + const out = []; + const linkPattern = /\]\(([^()\s#]+)/g; + let m; + while ((m = linkPattern.exec(text)) !== null) { + out.push(m[1]); + } + return out; + }; + + let changedLinks = 0; + for (const skill of ['react-patterns', 'react-performance', 'react-testing']) { + const sourceRel = `skills/${skill}/SKILL.md`; + const content = fs.readFileSync(path.join(REPO_ROOT, sourceRel), 'utf8'); + assert.ok(content.includes('](../../rules/'), `${sourceRel} should have a broken link pre-fix`); + const rewritten = rewriteRelativeLinks(content, { sourceRel, index: realIndex }); + assert.ok(!rewritten.includes('](../../rules/'), `${sourceRel} still has the broken depth`); + + // Only links we actually changed are validated here; cross-skill links to + // skills outside this module subset are legitimately left untouched. + const before = extractLinks(content); + const after = extractLinks(rewritten); + const installedSkillDir = path.posix.dirname(`skills/ecc/${skill}/SKILL.md`); + for (let i = 0; i < after.length; i += 1) { + if (after[i] === before[i]) { + continue; + } + const resolved = path.posix + .normalize(path.posix.join(installedSkillDir, after[i])) + .replace(/\/+$/, ''); + const isFile = installedDestRels.has(resolved); + const isDir = [...installedDestRels].some(d => d.startsWith(`${resolved}/`)); + assert.ok(isFile || isDir, `rewritten link ${after[i]} -> ${resolved} is not installed`); + changedLinks += 1; + } + } + assert.ok(changedLinks >= 3, `expected to verify rewritten links, changed ${changedLinks}`); + })) passed++; else failed++; + + console.log(`\nResults: Passed: ${passed}, Failed: ${failed}`); + process.exit(failed > 0 ? 1 : 0); +} + +runTests();