From 3db9d5405b1ffd5b7bb449aabecdaf14e6b05adc Mon Sep 17 00:00:00 2001 From: Pierre Brisorgueil Date: Tue, 4 Aug 2026 14:30:41 +0200 Subject: [PATCH 1/4] fix(public): resolve duplicate guide slugs by precedence, not scan order An application-level guide (any module other than home) now deterministically overrides a framework-provided (home) guide on a slug collision, silently. A same-tier collision (two framework or two application guides) keeps a deterministic tiebreak and warns with the actual remedy instead of "rename one" as the only option. buildDocsTree and the bySlug index are now built from the SAME deduped entry list (resolveGuideEntries), so the listing (/api/public/docs) and the fetch endpoint (/api/public/docs/:slug.md) can never disagree, and the tree never shows a duplicate nav entry. Closes #3979 --- modules/public/helpers/public.docs.tree.js | 87 ++++++++++++ .../public/services/public.docs.service.js | 16 +-- .../tests/public.docs.integration.tests.js | 59 ++++++++ ...public.docs.service.fallback.unit.tests.js | 1 + .../tests/public.docs.service.unit.tests.js | 33 +++-- .../tests/public.docs.tree.unit.tests.js | 130 +++++++++++++++++- 6 files changed, 305 insertions(+), 21 deletions(-) diff --git a/modules/public/helpers/public.docs.tree.js b/modules/public/helpers/public.docs.tree.js index 8a80afb56..aa621891a 100644 --- a/modules/public/helpers/public.docs.tree.js +++ b/modules/public/helpers/public.docs.tree.js @@ -19,6 +19,12 @@ * every audience ({@link DEFAULT_PERSONA}). A guide whose prefix falls outside * every configured range (or when no sections are configured) is grouped under * its capitalised module name so it is never silently dropped. + * + * A slug collision between two guide files is resolved by {@link resolveGuideEntries} + * BEFORE the tree is built, using a precedence policy (application-level guide + * overrides a framework-provided one) — see that function for details. The + * caller (the public docs service) runs the same deduped list through both + * `buildDocsTree` and its slug index, so the two endpoints can never disagree. */ import fs from 'fs'; import path from 'path'; @@ -182,6 +188,84 @@ const loadGuideEntries = (filePaths) => { .sort((a, b) => a.order - b.order || a.slug.localeCompare(b.slug)); }; +/** + * Name of the module whose guides ship as framework-provided demo content. + * Every other module's guides are application-level — added by the downstream + * consumer project (either a wholly new module, or extra guides dropped into + * an existing module's `doc/guides/`). Framework guides ship exclusively + * under `modules/home/doc/guides` today (see the two shipped samples); keying + * precedence off that module — rather than adding a config flag — grounds the + * distinction in data {@link loadGuideEntries} already produces (`entry.path`). + * @type {string} + */ +const FRAMEWORK_GUIDE_MODULE = 'home'; + +/** + * Precedence rank per tier — higher wins a cross-tier collision. + * @type {{ framework: 0, application: 1 }} + */ +const PRECEDENCE_RANK = { framework: 0, application: 1 }; + +/** + * Precedence tier for a guide entry on a slug collision. + * @param {{ path: string }} entry - Guide entry (from {@link loadGuideEntries}). + * @returns {'framework'|'application'} Precedence tier. + */ +const precedenceTier = (entry) => (moduleFromPath(entry.path) === FRAMEWORK_GUIDE_MODULE ? 'framework' : 'application'); + +/** + * Resolve slug collisions across guide entries by precedence, producing the + * single deduped list that feeds BOTH `buildDocsTree` and the service's slug + * index — so the listing and the fetch endpoint can never disagree, and the + * outcome no longer depends on file-scan order. + * + * Policy: + * 1. **Cross-tier collision** — an application-level guide (any module other + * than {@link FRAMEWORK_GUIDE_MODULE}) always overrides a framework-provided + * one, regardless of scan order. This is the supported override mechanism + * for a consumer that ships its own `00-welcome`/`01-quickstart`; it is + * silent (debug-logged only), not an error. + * 2. **Same-tier collision** (two framework guides, or two application + * guides) — precedence cannot disambiguate, so the kept guide is decided + * by {@link loadGuideEntries}'s existing deterministic sort (numeric + * prefix, then slug): the later entry in that order wins, same as before + * this fix. A warning is logged naming the actual policy, since neither + * guide "wins" by design. + * + * @param {ReturnType} entries - Structured guides, + * already sorted by {@link loadGuideEntries}. + * @returns {ReturnType} Entries with slug collisions + * resolved — at most one entry per slug, original relative order preserved. + */ +const resolveGuideEntries = (entries) => { + const list = Array.isArray(entries) ? entries : []; + const winners = new Map(); // slug -> winning entry + for (const entry of list) { + const incumbent = winners.get(entry.slug); + if (!incumbent) { + winners.set(entry.slug, entry); + continue; + } + const incumbentTier = precedenceTier(incumbent); + const challengerTier = precedenceTier(entry); + if (incumbentTier === challengerTier) { + logger.warn( + `[public/docs] duplicate guide slug "${entry.slug}" between two ${challengerTier} guides ` + + `("${incumbent.path}" vs "${entry.path}") — same precedence tier, so the kept guide is decided by file order, ` + + 'not policy; rename one guide, or move it to a module with different precedence (application overrides framework), to resolve deliberately.', + ); + winners.set(entry.slug, entry); // later entry wins — existing deterministic tiebreak + continue; + } + if (PRECEDENCE_RANK[challengerTier] > PRECEDENCE_RANK[incumbentTier]) { + logger.debug(`[public/docs] guide slug "${entry.slug}" — application guide (${entry.path}) overrides framework guide (${incumbent.path})`); + winners.set(entry.slug, entry); + } + // else: challenger is framework, incumbent is application — incumbent already wins. + } + return list.filter((entry) => winners.get(entry.slug) === entry); +}; + /** * Slugify a label into a URL-safe category id. * @param {string} label - Human category label. @@ -275,6 +359,7 @@ const buildDocsTree = (entries, sections = []) => { export default { DEFAULT_PERSONA, + FRAMEWORK_GUIDE_MODULE, slugFromPath, prefixFromPath, moduleFromPath, @@ -282,5 +367,7 @@ export default { stripLeadingH1, firstParagraph, loadGuideEntries, + precedenceTier, + resolveGuideEntries, buildDocsTree, }; diff --git a/modules/public/services/public.docs.service.js b/modules/public/services/public.docs.service.js index 901ebb1f4..77b8379c8 100644 --- a/modules/public/services/public.docs.service.js +++ b/modules/public/services/public.docs.service.js @@ -38,19 +38,17 @@ const guideFiles = () => (Array.isArray(config.files?.guides) ? [...config.files const guideSections = () => (Array.isArray(config.docs?.guideSections) ? config.docs.guideSections.map((s) => ({ ...s })) : []); /** - * @desc Build the docs tree + slug index from disk. + * @desc Build the docs tree + slug index from disk. Slug collisions are + * resolved once, up front, by {@link docsTree.resolveGuideEntries} (precedence + * policy: application-level guide overrides a framework-provided one). Both the + * tree and the slug index are built from that SAME deduped entry list, so the + * listing (`getTree`) and the fetch endpoint (`getMarkdown`) can never disagree. * @returns {{ tree: { categories: Object[] }, bySlug: Map }} */ const compute = () => { - const entries = docsTree.loadGuideEntries(guideFiles()); + const entries = docsTree.resolveGuideEntries(docsTree.loadGuideEntries(guideFiles())); const tree = docsTree.buildDocsTree(entries, guideSections()); - const bySlug = new Map(); - for (const entry of entries) { - if (bySlug.has(entry.slug)) { - logger.warn(`[public/docs] duplicate guide slug "${entry.slug}" — later guide wins; rename one to avoid the collision`); - } - bySlug.set(entry.slug, entry); - } + const bySlug = new Map(entries.map((entry) => [entry.slug, entry])); return { tree, bySlug }; }; diff --git a/modules/public/tests/public.docs.integration.tests.js b/modules/public/tests/public.docs.integration.tests.js index f55fcf12e..a88cc23cf 100644 --- a/modules/public/tests/public.docs.integration.tests.js +++ b/modules/public/tests/public.docs.integration.tests.js @@ -8,6 +8,8 @@ */ import request from 'supertest'; import path from 'path'; +import fs from 'fs'; +import os from 'os'; import { afterAll, beforeAll, beforeEach, describe, test, expect, @@ -112,3 +114,60 @@ describe('Public docs integration tests:', () => { expect(result.text.trimStart().startsWith('---')).toBe(false); }); }); + +describe('Public docs integration tests — slug-collision precedence:', () => { + let app; + let PublicDocsService; + let tmpDir; + let appGuidePath; + const originalOrgEnabled = config.organizations?.enabled; + const originalGuides = Array.isArray(config.files?.guides) ? [...config.files.guides] : []; + + beforeAll(async () => { + if (config.organizations) config.organizations.enabled = false; + const init = await bootstrap(); + app = init.app; + PublicDocsService = (await import(path.resolve('./modules/public/services/public.docs.service.js'))).default; + + // A real on-disk fixture guide, under a module path other than "home" — + // simulates a consumer/application module shipping its own "welcome" + // guide, colliding with the framework-shipped modules/home/00-welcome.md. + // Path substring only needs to contain "modules//", it does not + // need to live under the app's real modules/ directory. + tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'public-docs-precedence-')); + const appModuleDir = path.join(tmpDir, 'modules', 'app-fixture', 'doc', 'guides'); + fs.mkdirSync(appModuleDir, { recursive: true }); + appGuidePath = path.join(appModuleDir, '00-welcome.md'); + fs.writeFileSync(appGuidePath, '# App Welcome\n\nApplication-level welcome guide.\n'); + + config.files.guides = [...originalGuides, appGuidePath]; + }); + + afterAll(async () => { + if (config.organizations) config.organizations.enabled = originalOrgEnabled; + config.files.guides = originalGuides; + if (tmpDir) fs.rmSync(tmpDir, { recursive: true, force: true }); + await mongooseService.disconnect(); + }); + + beforeEach(() => { + if (PublicDocsService) PublicDocsService.clearCache(); + }); + + test('the application guide wins the "welcome" slug in the listing (no duplicate)', async () => { + const result = await request(app).get('/api/public/docs').expect(200); + const { categories } = result.body.data; + const guides = categories.flatMap((c) => c.guides); + const welcomeGuides = guides.filter((g) => g.slug === 'welcome'); + + // Exactly one "welcome" entry — the listing never shows a duplicate. + expect(welcomeGuides).toHaveLength(1); + expect(welcomeGuides[0].title).toBe('App Welcome'); + }); + + test('the application guide wins the "welcome" slug on fetch — listing and fetch agree', async () => { + const result = await request(app).get('/api/public/docs/welcome.md').expect(200); + expect(result.text).toContain('Application-level welcome guide.'); + expect(result.text).not.toContain('Welcome to'); + }); +}); diff --git a/modules/public/tests/public.docs.service.fallback.unit.tests.js b/modules/public/tests/public.docs.service.fallback.unit.tests.js index 64364aa94..2736f48a1 100644 --- a/modules/public/tests/public.docs.service.fallback.unit.tests.js +++ b/modules/public/tests/public.docs.service.fallback.unit.tests.js @@ -21,6 +21,7 @@ jest.unstable_mockModule('../../../lib/services/logger.js', () => ({ jest.unstable_mockModule('../helpers/public.docs.tree.js', () => ({ default: { loadGuideEntries: jest.fn().mockReturnValue([]), + resolveGuideEntries: jest.fn((entries) => entries), buildDocsTree: jest.fn().mockReturnValue({ categories: [] }), }, })); diff --git a/modules/public/tests/public.docs.service.unit.tests.js b/modules/public/tests/public.docs.service.unit.tests.js index f07deb81d..f96b0c683 100644 --- a/modules/public/tests/public.docs.service.unit.tests.js +++ b/modules/public/tests/public.docs.service.unit.tests.js @@ -2,8 +2,11 @@ * Unit tests for the public docs service. * Verifies tree assembly (driven by config.docs.guideSections), slug lookup, * the 404 (null) path, and TTL caching — with config + the docs-tree helper - * mocked so the test never touches disk. (The tree parsing itself is exercised - * in public.docs.tree.unit.tests.js.) + * mocked so the test never touches disk. (The tree parsing itself, including + * the slug-collision precedence policy, is exercised in + * public.docs.tree.unit.tests.js — this file only verifies that the service + * wires `loadGuideEntries` → `resolveGuideEntries` → both `buildDocsTree` and + * `bySlug`, so the two endpoints are built from the SAME deduped list.) */ import { jest, describe, test, expect, beforeEach, @@ -34,9 +37,10 @@ jest.unstable_mockModule('../../../lib/services/logger.js', () => ({ const loadGuideEntries = jest.fn(); const buildDocsTree = jest.fn(); +const resolveGuideEntries = jest.fn(); jest.unstable_mockModule('../helpers/public.docs.tree.js', () => ({ - default: { loadGuideEntries, buildDocsTree }, + default: { loadGuideEntries, buildDocsTree, resolveGuideEntries }, })); const PublicDocsService = (await import('../services/public.docs.service.js')).default; @@ -56,12 +60,16 @@ describe('PublicDocsService', () => { jest.clearAllMocks(); PublicDocsService.clearCache(); loadGuideEntries.mockReturnValue(sampleEntries); + // Pass-through by default — collision resolution itself is unit-tested in + // public.docs.tree.unit.tests.js; here we only verify the wiring. + resolveGuideEntries.mockImplementation((entries) => entries); buildDocsTree.mockReturnValue(sampleTree); }); - test('getTree builds the tree from the configured guide files + sections', () => { + test('getTree builds the tree from the configured guide files + sections, via resolveGuideEntries', () => { const tree = PublicDocsService.getTree(); expect(loadGuideEntries).toHaveBeenCalledWith(guideFilesPaths); + expect(resolveGuideEntries).toHaveBeenCalledWith(sampleEntries); expect(buildDocsTree).toHaveBeenCalledWith(sampleEntries, guideSections); expect(tree).toBe(sampleTree); }); @@ -92,8 +100,8 @@ describe('PublicDocsService', () => { expect(loadGuideEntries).toHaveBeenCalledTimes(2); }); - test('warns and last-wins when two entries share the same slug', () => { - const dupeEntries = [ + test('bySlug is built from resolveGuideEntries output, not the raw loaded entries — tree and slug index cannot disagree', () => { + const rawEntries = [ { slug: 'quickstart', title: 'Quickstart A', order: 0, summary: 'a', body: 'Body A', }, @@ -101,14 +109,17 @@ describe('PublicDocsService', () => { slug: 'quickstart', title: 'Quickstart B', order: 1, summary: 'b', body: 'Body B', }, ]; - loadGuideEntries.mockReturnValueOnce(dupeEntries); + // Collision already resolved upstream — resolveGuideEntries returns a + // single winner, same as the real helper would. + const deduped = [rawEntries[1]]; + loadGuideEntries.mockReturnValueOnce(rawEntries); + resolveGuideEntries.mockReturnValueOnce(deduped); PublicDocsService.getTree(); - expect(mockLogger.warn).toHaveBeenCalledWith( - expect.stringContaining('duplicate guide slug "quickstart"'), - ); - // Last entry wins + // buildDocsTree (the listing) receives the deduped list, not the raw one. + expect(buildDocsTree).toHaveBeenCalledWith(deduped, guideSections); + // bySlug (the fetch endpoint) agrees with the winner buildDocsTree saw. expect(PublicDocsService.getMarkdown('quickstart')).toBe('Body B'); }); }); diff --git a/modules/public/tests/public.docs.tree.unit.tests.js b/modules/public/tests/public.docs.tree.unit.tests.js index c74e4e1af..57e65a2d5 100644 --- a/modules/public/tests/public.docs.tree.unit.tests.js +++ b/modules/public/tests/public.docs.tree.unit.tests.js @@ -14,11 +14,15 @@ import fs from 'fs'; import os from 'os'; import path from 'path'; +import { jest } from '@jest/globals'; + import docsTree from '../helpers/public.docs.tree.js'; +import logger from '../../../lib/services/logger.js'; const { slugFromPath, prefixFromPath, moduleFromPath, titleFromMarkdown, - firstParagraph, loadGuideEntries, buildDocsTree, DEFAULT_PERSONA, + firstParagraph, loadGuideEntries, buildDocsTree, resolveGuideEntries, + precedenceTier, FRAMEWORK_GUIDE_MODULE, DEFAULT_PERSONA, } = docsTree; const sections = [ @@ -136,6 +140,130 @@ describe('loadGuideEntries:', () => { }); }); +describe('precedenceTier:', () => { + it(`classifies a guide from modules/${FRAMEWORK_GUIDE_MODULE} as framework`, () => { + expect(precedenceTier({ path: `modules/${FRAMEWORK_GUIDE_MODULE}/doc/guides/00-welcome.md` })).toBe('framework'); + }); + + it('classifies a guide from any other module as application', () => { + expect(precedenceTier({ path: 'modules/scrap/doc/guides/01-welcome.md' })).toBe('application'); + expect(precedenceTier({ path: 'modules/users/doc/guides/01-welcome.md' })).toBe('application'); + }); + + it('classifies a guide with no resolvable module path as application', () => { + expect(precedenceTier({ path: '/tmp/loose.md' })).toBe('application'); + }); +}); + +describe('resolveGuideEntries:', () => { + let warnSpy; + let debugSpy; + + beforeEach(() => { + warnSpy = jest.spyOn(logger, 'warn').mockImplementation(() => {}); + debugSpy = jest.spyOn(logger, 'debug').mockImplementation(() => {}); + }); + + afterEach(() => { + warnSpy.mockRestore(); + debugSpy.mockRestore(); + }); + + it('returns entries unchanged (no collision)', () => { + const entries = [ + { + slug: 'welcome', title: 'Welcome', order: 0, summary: 's', body: 'b', path: 'modules/home/doc/guides/00-welcome.md', + }, + { + slug: 'quickstart', title: 'Quickstart', order: 1, summary: 's', body: 'b', path: 'modules/home/doc/guides/01-quickstart.md', + }, + ]; + expect(resolveGuideEntries(entries)).toEqual(entries); + expect(warnSpy).not.toHaveBeenCalled(); + expect(debugSpy).not.toHaveBeenCalled(); + }); + + it('an application guide overrides a framework guide on the same slug (app wins regardless of scan order)', () => { + const frameworkGuide = { + slug: 'welcome', title: 'Framework Welcome', order: 0, summary: 's', body: 'framework body', path: 'modules/home/doc/guides/00-welcome.md', + }; + const appGuide = { + slug: 'welcome', title: 'App Welcome', order: 0, summary: 's', body: 'app body', path: 'modules/scrap/doc/guides/00-welcome.md', + }; + + // Framework guide scanned first, app guide second — app should still win. + const resolvedA = resolveGuideEntries([frameworkGuide, appGuide]); + expect(resolvedA).toHaveLength(1); + expect(resolvedA[0]).toBe(appGuide); + + // App guide scanned first, framework guide second — app should still win + // (precedence is order-independent). + const resolvedB = resolveGuideEntries([appGuide, frameworkGuide]); + expect(resolvedB).toHaveLength(1); + expect(resolvedB[0]).toBe(appGuide); + + // The override is silent (debug only), never a warning. + expect(warnSpy).not.toHaveBeenCalled(); + expect(debugSpy).toHaveBeenCalledWith(expect.stringContaining('overrides framework guide')); + }); + + it('same-tier collision (two framework guides) keeps the later entry and warns with the actual policy', () => { + const first = { + slug: 'quickstart', title: 'Quickstart A', order: 0, summary: 'a', body: 'Body A', path: 'modules/home/doc/guides/01-quickstart.md', + }; + const second = { + slug: 'quickstart', title: 'Quickstart B', order: 1, summary: 'b', body: 'Body B', path: 'modules/home/doc/guides/01b-quickstart.md', + }; + + const resolved = resolveGuideEntries([first, second]); + expect(resolved).toEqual([second]); + expect(warnSpy).toHaveBeenCalledWith( + expect.stringContaining('duplicate guide slug "quickstart" between two framework guides'), + ); + // The warning states the actual remedy, not just "rename one". + expect(warnSpy).toHaveBeenCalledWith(expect.stringContaining('different precedence')); + }); + + it('same-tier collision (two application guides) keeps the later entry and warns', () => { + const first = { + slug: 'setup', title: 'Setup A', order: 0, summary: 'a', body: 'A', path: 'modules/scrap/doc/guides/01-setup.md', + }; + const second = { + slug: 'setup', title: 'Setup B', order: 1, summary: 'b', body: 'B', path: 'modules/wizard/doc/guides/01-setup.md', + }; + + const resolved = resolveGuideEntries([first, second]); + expect(resolved).toEqual([second]); + expect(warnSpy).toHaveBeenCalledWith( + expect.stringContaining('duplicate guide slug "setup" between two application guides'), + ); + }); + + it('preserves relative order and dedupes independent of position', () => { + const home = { + slug: 'welcome', title: 'Home Welcome', order: 0, summary: 's', body: 'b', path: 'modules/home/doc/guides/00-welcome.md', + }; + const quickstart = { + slug: 'quickstart', title: 'Quickstart', order: 1, summary: 's', body: 'b', path: 'modules/home/doc/guides/01-quickstart.md', + }; + const appWelcome = { + slug: 'welcome', title: 'App Welcome', order: 0, summary: 's', body: 'b', path: 'modules/scrap/doc/guides/00-welcome.md', + }; + + const resolved = resolveGuideEntries([home, quickstart, appWelcome]); + // appWelcome wins the "welcome" slug, but the surviving entry keeps its + // OWN position in the input list (index 2) — home's entry is dropped + // from that position, quickstart (index 1) is unaffected. + expect(resolved.map((e) => e.slug)).toEqual(['quickstart', 'welcome']); + expect(resolved[1]).toBe(appWelcome); + }); + + it('returns [] for empty/invalid input', () => { + expect(resolveGuideEntries([])).toEqual([]); + expect(resolveGuideEntries(null)).toEqual([]); + }); +}); + describe('buildDocsTree:', () => { const entries = [ { From c385cd654be9ad778ec40e36b1a9c16427b2a68d Mon Sep 17 00:00:00 2001 From: Pierre Brisorgueil Date: Tue, 4 Aug 2026 14:39:08 +0200 Subject: [PATCH 2/4] refactor(simplify): reuse CORE_MODULES for guide precedence, drop rank map MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - precedenceTier now reuses the stack's existing framework-module set (lib/helpers/config.js CORE_MODULES) instead of a new single-module constant — same classification filterByActivation already relies on. - Drop PRECEDENCE_RANK: with only two tiers and same-tier collisions already handled earlier, the remaining branch is just "challenger is application". --- modules/public/helpers/public.docs.tree.js | 43 ++++++++----------- .../tests/public.docs.tree.unit.tests.js | 18 +++++--- 2 files changed, 31 insertions(+), 30 deletions(-) diff --git a/modules/public/helpers/public.docs.tree.js b/modules/public/helpers/public.docs.tree.js index aa621891a..c6cc1284b 100644 --- a/modules/public/helpers/public.docs.tree.js +++ b/modules/public/helpers/public.docs.tree.js @@ -30,6 +30,7 @@ import fs from 'fs'; import path from 'path'; import logger from '../../../lib/services/logger.js'; +import configHelper from '../../../lib/helpers/config.js'; /** * Default persona audience applied when a section declares none. @@ -189,29 +190,20 @@ const loadGuideEntries = (filePaths) => { }; /** - * Name of the module whose guides ship as framework-provided demo content. - * Every other module's guides are application-level — added by the downstream - * consumer project (either a wholly new module, or extra guides dropped into - * an existing module's `doc/guides/`). Framework guides ship exclusively - * under `modules/home/doc/guides` today (see the two shipped samples); keying - * precedence off that module — rather than adding a config flag — grounds the - * distinction in data {@link loadGuideEntries} already produces (`entry.path`). - * @type {string} - */ -const FRAMEWORK_GUIDE_MODULE = 'home'; - -/** - * Precedence rank per tier — higher wins a cross-tier collision. - * @type {{ framework: 0, application: 1 }} - */ -const PRECEDENCE_RANK = { framework: 0, application: 1 }; - -/** - * Precedence tier for a guide entry on a slug collision. + * Precedence tier for a guide entry on a slug collision. A module in + * `configHelper.CORE_MODULES` (`core`/`auth`/`users`/`home` — the stack's + * existing "ships with the framework, never deactivated" classification, + * see `lib/helpers/config.js`) is framework-provided; every other module is + * application-level — added by the downstream consumer project (either a + * wholly new module, or extra guides dropped into an existing module's + * `doc/guides/`). Guides ship exclusively under `modules/home/doc/guides` + * today (see the two shipped samples), so in practice this currently + * resolves to "home vs. everything else" — but it reuses the stack's single + * existing framework-module set rather than declaring a second, narrower one. * @param {{ path: string }} entry - Guide entry (from {@link loadGuideEntries}). * @returns {'framework'|'application'} Precedence tier. */ -const precedenceTier = (entry) => (moduleFromPath(entry.path) === FRAMEWORK_GUIDE_MODULE ? 'framework' : 'application'); +const precedenceTier = (entry) => (configHelper.CORE_MODULES.has(moduleFromPath(entry.path)) ? 'framework' : 'application'); /** * Resolve slug collisions across guide entries by precedence, producing the @@ -220,9 +212,9 @@ const precedenceTier = (entry) => (moduleFromPath(entry.path) === FRAMEWORK_GUID * outcome no longer depends on file-scan order. * * Policy: - * 1. **Cross-tier collision** — an application-level guide (any module other - * than {@link FRAMEWORK_GUIDE_MODULE}) always overrides a framework-provided - * one, regardless of scan order. This is the supported override mechanism + * 1. **Cross-tier collision** — an application-level guide (per + * {@link precedenceTier}) always overrides a framework-provided one, + * regardless of scan order. This is the supported override mechanism * for a consumer that ships its own `00-welcome`/`01-quickstart`; it is * silent (debug-logged only), not an error. * 2. **Same-tier collision** (two framework guides, or two application @@ -257,7 +249,9 @@ const resolveGuideEntries = (entries) => { winners.set(entry.slug, entry); // later entry wins — existing deterministic tiebreak continue; } - if (PRECEDENCE_RANK[challengerTier] > PRECEDENCE_RANK[incumbentTier]) { + if (challengerTier === 'application') { + // incumbentTier must be 'framework' here — the equal-tier case already + // returned above, and 'application'/'framework' are the only two tiers. logger.debug(`[public/docs] guide slug "${entry.slug}" — application guide (${entry.path}) overrides framework guide (${incumbent.path})`); winners.set(entry.slug, entry); } @@ -359,7 +353,6 @@ const buildDocsTree = (entries, sections = []) => { export default { DEFAULT_PERSONA, - FRAMEWORK_GUIDE_MODULE, slugFromPath, prefixFromPath, moduleFromPath, diff --git a/modules/public/tests/public.docs.tree.unit.tests.js b/modules/public/tests/public.docs.tree.unit.tests.js index 57e65a2d5..1a7427484 100644 --- a/modules/public/tests/public.docs.tree.unit.tests.js +++ b/modules/public/tests/public.docs.tree.unit.tests.js @@ -18,13 +18,16 @@ import { jest } from '@jest/globals'; import docsTree from '../helpers/public.docs.tree.js'; import logger from '../../../lib/services/logger.js'; +import configHelper from '../../../lib/helpers/config.js'; const { slugFromPath, prefixFromPath, moduleFromPath, titleFromMarkdown, firstParagraph, loadGuideEntries, buildDocsTree, resolveGuideEntries, - precedenceTier, FRAMEWORK_GUIDE_MODULE, DEFAULT_PERSONA, + precedenceTier, DEFAULT_PERSONA, } = docsTree; +const { CORE_MODULES } = configHelper; + const sections = [ { title: 'Get Started', prefixMin: 0, prefixMax: 1 }, { title: 'Guides', prefixMin: 2, prefixMax: 9, persona: ['agent'] }, @@ -141,13 +144,18 @@ describe('loadGuideEntries:', () => { }); describe('precedenceTier:', () => { - it(`classifies a guide from modules/${FRAMEWORK_GUIDE_MODULE} as framework`, () => { - expect(precedenceTier({ path: `modules/${FRAMEWORK_GUIDE_MODULE}/doc/guides/00-welcome.md` })).toBe('framework'); + it('classifies a guide from every CORE_MODULES entry as framework (reuses the stack\'s existing framework-module set)', () => { + for (const mod of CORE_MODULES) { + expect(precedenceTier({ path: `modules/${mod}/doc/guides/00-welcome.md` })).toBe('framework'); + } + // Sanity: "home" — the only module that ships guides today — is one of them. + expect(CORE_MODULES.has('home')).toBe(true); }); - it('classifies a guide from any other module as application', () => { + it('classifies a guide from a non-core module as application', () => { expect(precedenceTier({ path: 'modules/scrap/doc/guides/01-welcome.md' })).toBe('application'); - expect(precedenceTier({ path: 'modules/users/doc/guides/01-welcome.md' })).toBe('application'); + expect(precedenceTier({ path: 'modules/billing/doc/guides/01-welcome.md' })).toBe('application'); + expect(CORE_MODULES.has('billing')).toBe(false); }); it('classifies a guide with no resolvable module path as application', () => { From 36eb08cac733a6423b06382fe13f6d22a9ae5383 Mon Sep 17 00:00:00 2001 From: Pierre Brisorgueil Date: Tue, 4 Aug 2026 15:09:39 +0200 Subject: [PATCH 3/4] test(public): prove app-guide precedence in both scan-order directions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The welcome-slug fixture used an absolute OS-tmpdir path, which always sorts before the relative modules/home/... path loadGuideEntries scans — so it only proved the override when the app guide is scanned FIRST. Add a second fixture (quickstart slug) at a relative path that sorts AFTER modules/home/..., proving the override also holds when the app guide is scanned LAST — end-to-end, not just via the mocked orderings already covered in public.docs.tree.unit.tests.js. Addresses CodeRabbit review on PR #4011. --- .../tests/public.docs.integration.tests.js | 39 +++++++++++++++++-- 1 file changed, 36 insertions(+), 3 deletions(-) diff --git a/modules/public/tests/public.docs.integration.tests.js b/modules/public/tests/public.docs.integration.tests.js index a88cc23cf..4851085e9 100644 --- a/modules/public/tests/public.docs.integration.tests.js +++ b/modules/public/tests/public.docs.integration.tests.js @@ -120,6 +120,8 @@ describe('Public docs integration tests — slug-collision precedence:', () => { let PublicDocsService; let tmpDir; let appGuidePath; + let reverseFixtureDir; + let reverseGuidePath; const originalOrgEnabled = config.organizations?.enabled; const originalGuides = Array.isArray(config.files?.guides) ? [...config.files.guides] : []; @@ -133,20 +135,37 @@ describe('Public docs integration tests — slug-collision precedence:', () => { // simulates a consumer/application module shipping its own "welcome" // guide, colliding with the framework-shipped modules/home/00-welcome.md. // Path substring only needs to contain "modules//", it does not - // need to live under the app's real modules/ directory. + // need to live under the app's real modules/ directory. loadGuideEntries + // sorts ALL guide paths alphabetically before assigning scan order, and an + // absolute OS-tmpdir path always sorts before the relative "modules/..." + // path config.files.guides uses for real guides — so this fixture is + // scanned BEFORE modules/home/00-welcome.md (app is the earlier entry). tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'public-docs-precedence-')); const appModuleDir = path.join(tmpDir, 'modules', 'app-fixture', 'doc', 'guides'); fs.mkdirSync(appModuleDir, { recursive: true }); appGuidePath = path.join(appModuleDir, '00-welcome.md'); fs.writeFileSync(appGuidePath, '# App Welcome\n\nApplication-level welcome guide.\n'); - config.files.guides = [...originalGuides, appGuidePath]; + // Second fixture, deliberately scanned in the OPPOSITE direction: a + // relative path (resolved against process.cwd(), the repo root under + // jest) starting with "zzz-" sorts AFTER "modules/home/...", so this one + // is scanned AFTER modules/home/01-quickstart.md (app is the LATER + // entry here). Proves the override is scan-order independent in both + // directions end-to-end, not just via the unit-level resolveGuideEntries + // tests (public.docs.tree.unit.tests.js) which mock the entry order. + reverseFixtureDir = path.join(process.cwd(), 'zzz-tmp-pr3979-fixture', 'doc', 'guides'); + fs.mkdirSync(reverseFixtureDir, { recursive: true }); + reverseGuidePath = path.join(reverseFixtureDir, '01-quickstart.md'); + fs.writeFileSync(reverseGuidePath, '# App Quickstart\n\nApplication-level quickstart guide.\n'); + + config.files.guides = [...originalGuides, appGuidePath, reverseGuidePath]; }); afterAll(async () => { if (config.organizations) config.organizations.enabled = originalOrgEnabled; config.files.guides = originalGuides; if (tmpDir) fs.rmSync(tmpDir, { recursive: true, force: true }); + if (reverseFixtureDir) fs.rmSync(path.join(process.cwd(), 'zzz-tmp-pr3979-fixture'), { recursive: true, force: true }); await mongooseService.disconnect(); }); @@ -154,7 +173,7 @@ describe('Public docs integration tests — slug-collision precedence:', () => { if (PublicDocsService) PublicDocsService.clearCache(); }); - test('the application guide wins the "welcome" slug in the listing (no duplicate)', async () => { + test('the application guide wins the "welcome" slug in the listing (no duplicate) — app scanned BEFORE the framework guide', async () => { const result = await request(app).get('/api/public/docs').expect(200); const { categories } = result.body.data; const guides = categories.flatMap((c) => c.guides); @@ -170,4 +189,18 @@ describe('Public docs integration tests — slug-collision precedence:', () => { expect(result.text).toContain('Application-level welcome guide.'); expect(result.text).not.toContain('Welcome to'); }); + + test('the application guide wins the "quickstart" slug — app scanned AFTER the framework guide (opposite scan-order direction)', async () => { + const result = await request(app).get('/api/public/docs').expect(200); + const { categories } = result.body.data; + const guides = categories.flatMap((c) => c.guides); + const quickstartGuides = guides.filter((g) => g.slug === 'quickstart'); + + expect(quickstartGuides).toHaveLength(1); + expect(quickstartGuides[0].title).toBe('App Quickstart'); + + const fetchResult = await request(app).get('/api/public/docs/quickstart.md').expect(200); + expect(fetchResult.text).toContain('Application-level quickstart guide.'); + expect(fetchResult.text).not.toContain(''); + }); }); From cca8e9ff922d2a35b116064323098c2a1e6fda9c Mon Sep 17 00:00:00 2001 From: Pierre Brisorgueil Date: Tue, 4 Aug 2026 15:34:52 +0200 Subject: [PATCH 4/4] fix(test): make the reverse-direction fixture genuinely relative MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit path.join(process.cwd(), ...) is always absolute, so the "opposite scan-order direction" fixture was in fact absolute too — both integration fixtures sorted before modules/home/..., meaning the app guide was the incumbent in both collisions and the explicit "challenger overrides incumbent" branch in resolveGuideEntries was never exercised at the integration level (verified by execution: a debug-spy showed zero calls, and reverting the source made both "opposite direction" assertions fail identically). Push a real relative path (path.relative(process.cwd(), ...)) into config.files.guides for the quickstart fixture instead, so it sorts AFTER modules/home/... and the framework guide is genuinely incumbent. Add a logger.debug spy proving the override branch fires only for that case, and scope the "welcome" test's assertion to its own slug (the same compute() pass also resolves the quickstart collision, so a blanket "not called" would be a false negative). --- .../tests/public.docs.integration.tests.js | 76 ++++++++++++++----- 1 file changed, 56 insertions(+), 20 deletions(-) diff --git a/modules/public/tests/public.docs.integration.tests.js b/modules/public/tests/public.docs.integration.tests.js index 4851085e9..88102ce61 100644 --- a/modules/public/tests/public.docs.integration.tests.js +++ b/modules/public/tests/public.docs.integration.tests.js @@ -12,11 +12,12 @@ import fs from 'fs'; import os from 'os'; import { - afterAll, beforeAll, beforeEach, describe, test, expect, + jest, afterAll, beforeAll, beforeEach, describe, test, expect, } from '@jest/globals'; import { bootstrap } from '../../../lib/app.js'; import mongooseService from '../../../lib/services/mongoose.js'; import config from '../../../config/index.js'; +import logger from '../../../lib/services/logger.js'; describe('Public docs integration tests:', () => { let app; @@ -118,10 +119,12 @@ describe('Public docs integration tests:', () => { describe('Public docs integration tests — slug-collision precedence:', () => { let app; let PublicDocsService; + let debugSpy; let tmpDir; let appGuidePath; - let reverseFixtureDir; - let reverseGuidePath; + let reverseFixtureAbsDir; + let reverseFixtureAbsPath; + let reverseGuideRelPath; const originalOrgEnabled = config.organizations?.enabled; const originalGuides = Array.isArray(config.files?.guides) ? [...config.files.guides] : []; @@ -130,6 +133,7 @@ describe('Public docs integration tests — slug-collision precedence:', () => { const init = await bootstrap(); app = init.app; PublicDocsService = (await import(path.resolve('./modules/public/services/public.docs.service.js'))).default; + debugSpy = jest.spyOn(logger, 'debug'); // A real on-disk fixture guide, under a module path other than "home" — // simulates a consumer/application module shipping its own "welcome" @@ -139,41 +143,52 @@ describe('Public docs integration tests — slug-collision precedence:', () => { // sorts ALL guide paths alphabetically before assigning scan order, and an // absolute OS-tmpdir path always sorts before the relative "modules/..." // path config.files.guides uses for real guides — so this fixture is - // scanned BEFORE modules/home/00-welcome.md (app is the earlier entry). + // scanned BEFORE modules/home/00-welcome.md (app is the earlier entry, + // already-incumbent when the framework guide arrives as challenger — the + // SILENT "incumbent already wins" branch in resolveGuideEntries, no + // logger.debug call). tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'public-docs-precedence-')); const appModuleDir = path.join(tmpDir, 'modules', 'app-fixture', 'doc', 'guides'); fs.mkdirSync(appModuleDir, { recursive: true }); appGuidePath = path.join(appModuleDir, '00-welcome.md'); fs.writeFileSync(appGuidePath, '# App Welcome\n\nApplication-level welcome guide.\n'); - // Second fixture, deliberately scanned in the OPPOSITE direction: a - // relative path (resolved against process.cwd(), the repo root under - // jest) starting with "zzz-" sorts AFTER "modules/home/...", so this one - // is scanned AFTER modules/home/01-quickstart.md (app is the LATER - // entry here). Proves the override is scan-order independent in both - // directions end-to-end, not just via the unit-level resolveGuideEntries - // tests (public.docs.tree.unit.tests.js) which mock the entry order. - reverseFixtureDir = path.join(process.cwd(), 'zzz-tmp-pr3979-fixture', 'doc', 'guides'); - fs.mkdirSync(reverseFixtureDir, { recursive: true }); - reverseGuidePath = path.join(reverseFixtureDir, '01-quickstart.md'); - fs.writeFileSync(reverseGuidePath, '# App Quickstart\n\nApplication-level quickstart guide.\n'); - - config.files.guides = [...originalGuides, appGuidePath, reverseGuidePath]; + // Second fixture, deliberately scanned in the OPPOSITE direction. The + // file is still written at an absolute path (fs calls need one), but the + // string pushed into config.files.guides below is made GENUINELY + // relative via path.relative() — path.join(process.cwd(), ...) is + // ALWAYS absolute (an earlier version of this test wrongly assumed + // otherwise, so both fixtures ended up absolute and this direction was + // never actually exercised — caught by review). A relative string + // starting with "zzz-" sorts AFTER "modules/home/...", so THIS fixture + // is scanned AFTER modules/home/01-quickstart.md: the framework guide is + // incumbent, the app guide is the LATER challenger that must explicitly + // override it — the logger.debug branch in resolveGuideEntries, asserted + // below. + reverseFixtureAbsDir = path.join(process.cwd(), 'zzz-tmp-pr3979-fixture', 'doc', 'guides'); + fs.mkdirSync(reverseFixtureAbsDir, { recursive: true }); + reverseFixtureAbsPath = path.join(reverseFixtureAbsDir, '01-quickstart.md'); + fs.writeFileSync(reverseFixtureAbsPath, '# App Quickstart\n\nApplication-level quickstart guide.\n'); + reverseGuideRelPath = path.relative(process.cwd(), reverseFixtureAbsPath); + + config.files.guides = [...originalGuides, appGuidePath, reverseGuideRelPath]; }); afterAll(async () => { if (config.organizations) config.organizations.enabled = originalOrgEnabled; config.files.guides = originalGuides; + if (debugSpy) debugSpy.mockRestore(); if (tmpDir) fs.rmSync(tmpDir, { recursive: true, force: true }); - if (reverseFixtureDir) fs.rmSync(path.join(process.cwd(), 'zzz-tmp-pr3979-fixture'), { recursive: true, force: true }); + if (reverseFixtureAbsDir) fs.rmSync(path.join(process.cwd(), 'zzz-tmp-pr3979-fixture'), { recursive: true, force: true }); await mongooseService.disconnect(); }); beforeEach(() => { if (PublicDocsService) PublicDocsService.clearCache(); + if (debugSpy) debugSpy.mockClear(); }); - test('the application guide wins the "welcome" slug in the listing (no duplicate) — app scanned BEFORE the framework guide', async () => { + test('the application guide wins the "welcome" slug in the listing (no duplicate) — app scanned BEFORE the framework guide (silent "incumbent already wins" branch)', async () => { const result = await request(app).get('/api/public/docs').expect(200); const { categories } = result.body.data; const guides = categories.flatMap((c) => c.guides); @@ -182,6 +197,15 @@ describe('Public docs integration tests — slug-collision precedence:', () => { // Exactly one "welcome" entry — the listing never shows a duplicate. expect(welcomeGuides).toHaveLength(1); expect(welcomeGuides[0].title).toBe('App Welcome'); + + // This direction wins because the app guide is already incumbent — it + // does NOT go through the explicit override branch (that's proven + // separately by the "quickstart" case below). Scope the assertion to + // "welcome" specifically: the same compute() pass also resolves the + // quickstart collision (both fixtures share one config.files.guides), + // which DOES log — asserting a blanket "not called" here would be a + // false negative. + expect(debugSpy).not.toHaveBeenCalledWith(expect.stringContaining('guide slug "welcome"')); }); test('the application guide wins the "welcome" slug on fetch — listing and fetch agree', async () => { @@ -190,7 +214,7 @@ describe('Public docs integration tests — slug-collision precedence:', () => { expect(result.text).not.toContain('Welcome to'); }); - test('the application guide wins the "quickstart" slug — app scanned AFTER the framework guide (opposite scan-order direction)', async () => { + test('the application guide overrides the framework guide for "quickstart" — app scanned AFTER it (opposite scan-order direction), proven via the explicit override branch firing', async () => { const result = await request(app).get('/api/public/docs').expect(200); const { categories } = result.body.data; const guides = categories.flatMap((c) => c.guides); @@ -202,5 +226,17 @@ describe('Public docs integration tests — slug-collision precedence:', () => { const fetchResult = await request(app).get('/api/public/docs/quickstart.md').expect(200); expect(fetchResult.text).toContain('Application-level quickstart guide.'); expect(fetchResult.text).not.toContain(''); + + // Proof this exercised the explicit "challenger overrides incumbent" + // branch in resolveGuideEntries (public.docs.tree.js) — that branch is + // the ONLY place this debug line is emitted, so its presence is direct + // evidence the framework guide was incumbent and the app guide, scanned + // later, actively overrode it (not merely "already winning"). + expect(debugSpy).toHaveBeenCalledWith( + expect.stringContaining('guide slug "quickstart"'), + ); + expect(debugSpy).toHaveBeenCalledWith( + expect.stringContaining('overrides framework guide'), + ); }); });