From e31a703bd787219b57de5a514ace79bb13e4af70 Mon Sep 17 00:00:00 2001 From: Rhuan Barreto Date: Sat, 13 Jun 2026 20:17:58 +0200 Subject: [PATCH 1/5] fix(import): improve error messages and extract business logic from command Replace the generic "Cannot detect import target" error with three context-aware messages: pack not found (with available packs list), path does not exist, and path exists but is not a valid target. Extract all business logic from src/commands/adr/import.ts into src/helpers/adr-import.ts (ARCH-001 compliance) and move the findProjectRoot() check inside the try-catch (ARCH-012 compliance). Closes #413 Signed-off-by: Rhuan Barreto --- src/commands/adr/import.ts | 318 +++---------------------------- src/helpers/adr-import.ts | 301 +++++++++++++++++++++++++++++ src/helpers/registry.ts | 53 +++++- tests/helpers/adr-import.test.ts | 295 ++++++++++++++++++++++++++++ tests/helpers/registry.test.ts | 36 +++- 5 files changed, 702 insertions(+), 301 deletions(-) create mode 100644 src/helpers/adr-import.ts create mode 100644 tests/helpers/adr-import.test.ts diff --git a/src/commands/adr/import.ts b/src/commands/adr/import.ts index 56e0aa36..4ef275af 100644 --- a/src/commands/adr/import.ts +++ b/src/commands/adr/import.ts @@ -1,25 +1,22 @@ // SPDX-License-Identifier: Apache-2.0 // Copyright 2026 Archgate -import { - existsSync, - mkdirSync, - rmSync, - unlinkSync, - writeFileSync, -} from "node:fs"; -import { basename, join } from "node:path"; +import { mkdirSync } from "node:fs"; import { styleText } from "node:util"; import type { Command } from "@commander-js/extra-typings"; -import { parseAdr } from "../../formats/adr"; import { - ImportsManifestSchema, - type ImportsManifest, -} from "../../formats/pack"; -import { getNextId, slugify } from "../../helpers/adr-writer"; + buildIdMap, + cleanupTempDirs, + collectAdrsToImport, + loadImportsManifest, + resolveAndCloneSources, + saveImportsManifest, + updateImportsManifest, + writeImportedAdrs, +} from "../../helpers/adr-import"; import { exitWith } from "../../helpers/exit"; -import { logDebug, logError } from "../../helpers/log"; +import { logError } from "../../helpers/log"; import { formatJSON, isAgentContext } from "../../helpers/output"; import { findProjectRoot } from "../../helpers/paths"; import { @@ -27,211 +24,8 @@ import { resolvedProjectPaths, } from "../../helpers/project-config"; import { withPromptFix } from "../../helpers/prompt"; -import { - resolveSource, - shallowClone, - detectTarget, - type ImportTarget, -} from "../../helpers/registry"; import { ensureRulesShim } from "../../helpers/rules-shim"; -// ---------- Imports manifest I/O ---------- - -async function loadImportsManifest( - projectRoot: string -): Promise { - const importsPath = join(projectRoot, ".archgate", "imports.json"); - if (!existsSync(importsPath)) return { imports: [] }; - const raw = await Bun.file(importsPath).json(); - return ImportsManifestSchema.parse(raw); -} - -function saveImportsManifest( - projectRoot: string, - manifest: ImportsManifest -): void { - const importsPath = join(projectRoot, ".archgate", "imports.json"); - writeFileSync(importsPath, JSON.stringify(manifest, null, 2) + "\n"); -} - -// ---------- ID rewriting ---------- - -function rewriteAdrId(content: string, _oldId: string, newId: string): string { - // Replace id in frontmatter YAML only (between --- delimiters). - // We extract the frontmatter block, replace the id line, and reconstruct. - const fmRegex = /^(---\r?\n)([\s\S]*?\r?\n)(---)/mu; - const match = content.match(fmRegex); - if (!match) return content; - - const [fullMatch, openDelim, fmBody, closeDelim] = match; - const updatedFm = fmBody.replace(/^(id:\s*).*$/mu, `$1${newId}`); - return content.replace(fullMatch, `${openDelim}${updatedFm}${closeDelim}`); -} - -// ---------- Helpers to avoid await-in-loop ---------- - -interface ResolvedImport { - source: string; - target: ImportTarget; - cloneDir: string; -} - -interface AdrToImport { - sourcePath: string; - rulesPath: string | null; - originalId: string; - title: string; - domain?: string; - source: string; - packVersion?: string; -} - -/** - * Resolve and clone all sources. Uses a cache to avoid re-cloning the same repo. - * Sequential because clone N may share a repo with clone N+1 (dedup). - */ -async function resolveAndCloneSources( - sources: string[] -): Promise<{ resolved: ResolvedImport[]; tempDirs: string[] }> { - const tempDirs: string[] = []; - const resolved: ResolvedImport[] = []; - const cloneCache = new Map(); - - for (const source of sources) { - const res = resolveSource(source); - logDebug("Resolved source:", JSON.stringify(res)); - - const cacheKey = `${res.repoUrl}#${res.ref ?? ""}`; - let cloneDir = cloneCache.get(cacheKey); - - if (!cloneDir) { - cloneDir = await shallowClone(res.repoUrl, res.ref); // eslint-disable-line no-await-in-loop -- sequential by design (dedup) - cloneCache.set(cacheKey, cloneDir); - tempDirs.push(cloneDir); - } - - const target = await detectTarget(cloneDir, res.subpath); // eslint-disable-line no-await-in-loop -- depends on prior clone - resolved.push({ source, target, cloneDir }); - } - - return { resolved, tempDirs }; -} - -/** - * Read all ADR files from resolved targets and build the import list. - */ -async function collectAdrsToImport( - resolved: ResolvedImport[] -): Promise { - const adrsToImport: AdrToImport[] = []; - - const readPromises: Array> = resolved.map( - async ({ source, target }) => { - const items: AdrToImport[] = []; - if (target.kind === "pack") { - const contents = await Promise.all( - target.adrFiles.map((f) => Bun.file(f).text()) - ); - for (let i = 0; i < target.adrFiles.length; i++) { - const adrFile = target.adrFiles[i]; - const content = contents[i]; - const adr = parseAdr(content, adrFile); - const adrBase = basename(adrFile, ".md"); - const rulesFile = target.rulesFiles.find( - (r) => basename(r, ".rules.ts") === adrBase - ); - items.push({ - sourcePath: adrFile, - rulesPath: rulesFile ?? null, - originalId: adr.frontmatter.id, - title: adr.frontmatter.title, - domain: adr.frontmatter.domain, - source, - packVersion: target.packMeta.version, - }); - } - } else { - const content = await Bun.file(target.adrFile).text(); - const adr = parseAdr(content, target.adrFile); - items.push({ - sourcePath: target.adrFile, - rulesPath: target.rulesFile, - originalId: adr.frontmatter.id, - title: adr.frontmatter.title, - domain: adr.frontmatter.domain, - source, - }); - } - return items; - } - ); - - const results = await Promise.all(readPromises); - for (const items of results) { - adrsToImport.push(...items); - } - return adrsToImport; -} - -/** - * Write imported ADR files to disk with remapped IDs. - * Returns list of written file paths for rollback on failure. - */ -async function writeImportedAdrs( - adrsToImport: AdrToImport[], - idMap: Array<{ original: string; newId: string; title: string }>, - adrsDir: string -): Promise { - const writtenFiles: string[] = []; - - // Read all source files in parallel first - const readTasks = adrsToImport.map((adr) => Bun.file(adr.sourcePath).text()); - const ruleTasks = adrsToImport.map((adr) => - adr.rulesPath ? Bun.file(adr.rulesPath).text() : Promise.resolve(null) - ); - const [contents, rulesContents] = await Promise.all([ - Promise.all(readTasks), - Promise.all(ruleTasks), - ]); - - try { - for (let i = 0; i < adrsToImport.length; i++) { - const adr = adrsToImport[i]; - const mapping = idMap[i]; - const slug = slugify(mapping.title); - const newFileName = `${mapping.newId}-${slug}.md`; - const destPath = join(adrsDir, newFileName); - - const rewritten = rewriteAdrId( - contents[i], - adr.originalId, - mapping.newId - ); - writeFileSync(destPath, rewritten); - writtenFiles.push(destPath); - - if (rulesContents[i] !== null) { - const rulesFileName = `${mapping.newId}-${slug}.rules.ts`; - const rulesDestPath = join(adrsDir, rulesFileName); - writeFileSync(rulesDestPath, rulesContents[i]!); - writtenFiles.push(rulesDestPath); - } - } - } catch (err) { - // Rollback: delete all written files - for (const file of writtenFiles) { - try { - unlinkSync(file); - } catch { - // Best effort - } - } - throw err; - } - - return writtenFiles; -} - // ---------- Command registration ---------- export function registerAdrImportCommand(adr: Command) { @@ -244,14 +38,14 @@ export function registerAdrImportCommand(adr: Command) { .option("--dry-run", "Preview changes without writing", false) .option("--list", "List previously imported ADRs", false) .action(async (sources, opts) => { - const projectRoot = findProjectRoot(); - if (!projectRoot) { - logError("No .archgate/ directory found. Run `archgate init` first."); - await exitWith(1); - return; - } - try { + const projectRoot = findProjectRoot(); + if (!projectRoot) { + logError("No .archgate/ directory found. Run `archgate init` first."); + await exitWith(1); + return; + } + const paths = resolvedProjectPaths(projectRoot); const useJson = opts.json || isAgentContext(); @@ -286,7 +80,7 @@ export function registerAdrImportCommand(adr: Command) { if (adrsToImport.length === 0) { console.log("No ADRs found to import."); - cleanup(tempDirs); + cleanupTempDirs(tempDirs); return; } @@ -294,35 +88,8 @@ export function registerAdrImportCommand(adr: Command) { mkdirSync(paths.adrsDir, { recursive: true }); - // Resolve each ADR's domain to the project's prefix for that domain. const domainPrefixes = getMergedDomainPrefixes(projectRoot); - - // Track the next available ID per prefix to avoid collisions - const nextIdByPrefix = new Map(); - - const idMap: Array<{ original: string; newId: string; title: string }> = - []; - - for (const adr of adrsToImport) { - const prefix = (adr.domain && domainPrefixes[adr.domain]) || "ARCH"; - - if (!nextIdByPrefix.has(prefix)) { - nextIdByPrefix.set(prefix, getNextId(paths.adrsDir, prefix)); - } - - const nextId = nextIdByPrefix.get(prefix)!; - idMap.push({ - original: adr.originalId, - newId: nextId, - title: adr.title, - }); - - const num = parseInt(nextId.replace(`${prefix}-`, ""), 10) + 1; - nextIdByPrefix.set( - prefix, - `${prefix}-${String(num).padStart(3, "0")}` - ); - } + const idMap = buildIdMap(adrsToImport, paths.adrsDir, domainPrefixes); // ---------- Preview ---------- @@ -365,7 +132,7 @@ export function registerAdrImportCommand(adr: Command) { } else { console.log("Dry run — no files written."); } - cleanup(tempDirs); + cleanupTempDirs(tempDirs); return; } @@ -385,7 +152,7 @@ export function registerAdrImportCommand(adr: Command) { ); if (!confirm) { console.log("Import cancelled."); - cleanup(tempDirs); + cleanupTempDirs(tempDirs); return; } } @@ -397,34 +164,7 @@ export function registerAdrImportCommand(adr: Command) { // ---------- Update imports.json ---------- const manifest = await loadImportsManifest(projectRoot); - const sourceGroups = new Map< - string, - { version?: string; ids: string[] } - >(); - - for (let i = 0; i < adrsToImport.length; i++) { - const adr = adrsToImport[i]; - const mapping = idMap[i]; - const existing = sourceGroups.get(adr.source); - if (existing) { - existing.ids.push(mapping.newId); - } else { - sourceGroups.set(adr.source, { - version: adr.packVersion, - ids: [mapping.newId], - }); - } - } - - for (const [source, group] of sourceGroups) { - manifest.imports.push({ - source, - version: group.version, - importedAt: new Date().toISOString(), - adrIds: group.ids, - }); - } - + updateImportsManifest(manifest, adrsToImport, idMap); saveImportsManifest(projectRoot, manifest); // ---------- Ensure rules.d.ts ---------- @@ -433,7 +173,7 @@ export function registerAdrImportCommand(adr: Command) { // ---------- Cleanup & summary ---------- - cleanup(tempDirs); + cleanupTempDirs(tempDirs); if (useJson) { console.log( @@ -463,13 +203,3 @@ export function registerAdrImportCommand(adr: Command) { } }); } - -function cleanup(dirs: string[]): void { - for (const dir of dirs) { - try { - rmSync(dir, { recursive: true, force: true }); - } catch { - logDebug("Failed to clean up temp dir:", dir); - } - } -} diff --git a/src/helpers/adr-import.ts b/src/helpers/adr-import.ts new file mode 100644 index 00000000..7ae0bede --- /dev/null +++ b/src/helpers/adr-import.ts @@ -0,0 +1,301 @@ +// SPDX-License-Identifier: Apache-2.0 +// Copyright 2026 Archgate +import { existsSync, rmSync, unlinkSync, writeFileSync } from "node:fs"; +import { basename, join } from "node:path"; + +import { parseAdr } from "../formats/adr"; +import { ImportsManifestSchema, type ImportsManifest } from "../formats/pack"; +import { getNextId, slugify } from "./adr-writer"; +import { logDebug } from "./log"; +import { + resolveSource, + shallowClone, + detectTarget, + type ImportTarget, +} from "./registry"; + +// ---------- Types ---------- + +export interface ResolvedImport { + source: string; + target: ImportTarget; + cloneDir: string; +} + +export interface AdrToImport { + sourcePath: string; + rulesPath: string | null; + originalId: string; + title: string; + domain?: string; + source: string; + packVersion?: string; +} + +export interface IdMapping { + original: string; + newId: string; + title: string; +} + +// ---------- Imports manifest I/O ---------- + +export async function loadImportsManifest( + projectRoot: string +): Promise { + const importsPath = join(projectRoot, ".archgate", "imports.json"); + if (!existsSync(importsPath)) return { imports: [] }; + const raw = await Bun.file(importsPath).json(); + return ImportsManifestSchema.parse(raw); +} + +export function saveImportsManifest( + projectRoot: string, + manifest: ImportsManifest +): void { + const importsPath = join(projectRoot, ".archgate", "imports.json"); + writeFileSync(importsPath, JSON.stringify(manifest, null, 2) + "\n"); +} + +// ---------- ID rewriting ---------- + +export function rewriteAdrId( + content: string, + _oldId: string, + newId: string +): string { + // Replace id in frontmatter YAML only (between --- delimiters). + // We extract the frontmatter block, replace the id line, and reconstruct. + const fmRegex = /^(---\r?\n)([\s\S]*?\r?\n)(---)/mu; + const match = content.match(fmRegex); + if (!match) return content; + + const [fullMatch, openDelim, fmBody, closeDelim] = match; + const updatedFm = fmBody.replace(/^(id:\s*).*$/mu, `$1${newId}`); + return content.replace(fullMatch, `${openDelim}${updatedFm}${closeDelim}`); +} + +// ---------- Source resolution & cloning ---------- + +/** + * Resolve and clone all sources. Uses a cache to avoid re-cloning the same repo. + * Sequential because clone N may share a repo with clone N+1 (dedup). + */ +export async function resolveAndCloneSources( + sources: string[] +): Promise<{ resolved: ResolvedImport[]; tempDirs: string[] }> { + const tempDirs: string[] = []; + const resolved: ResolvedImport[] = []; + const cloneCache = new Map(); + + for (const source of sources) { + const res = resolveSource(source); + logDebug("Resolved source:", JSON.stringify(res)); + + const cacheKey = `${res.repoUrl}#${res.ref ?? ""}`; + let cloneDir = cloneCache.get(cacheKey); + + if (!cloneDir) { + cloneDir = await shallowClone(res.repoUrl, res.ref); // eslint-disable-line no-await-in-loop -- sequential by design (dedup) + cloneCache.set(cacheKey, cloneDir); + tempDirs.push(cloneDir); + } + + const target = await detectTarget(cloneDir, res.subpath, res.kind); // eslint-disable-line no-await-in-loop -- depends on prior clone + resolved.push({ source, target, cloneDir }); + } + + return { resolved, tempDirs }; +} + +// ---------- ADR collection ---------- + +/** + * Read all ADR files from resolved targets and build the import list. + */ +export async function collectAdrsToImport( + resolved: ResolvedImport[] +): Promise { + const readPromises: Array> = resolved.map( + async ({ source, target }) => { + const items: AdrToImport[] = []; + if (target.kind === "pack") { + const contents = await Promise.all( + target.adrFiles.map((f) => Bun.file(f).text()) + ); + for (let i = 0; i < target.adrFiles.length; i++) { + const adrFile = target.adrFiles[i]; + const content = contents[i]; + const adr = parseAdr(content, adrFile); + const adrBase = basename(adrFile, ".md"); + const rulesFile = target.rulesFiles.find( + (r) => basename(r, ".rules.ts") === adrBase + ); + items.push({ + sourcePath: adrFile, + rulesPath: rulesFile ?? null, + originalId: adr.frontmatter.id, + title: adr.frontmatter.title, + domain: adr.frontmatter.domain, + source, + packVersion: target.packMeta.version, + }); + } + } else { + const content = await Bun.file(target.adrFile).text(); + const adr = parseAdr(content, target.adrFile); + items.push({ + sourcePath: target.adrFile, + rulesPath: target.rulesFile, + originalId: adr.frontmatter.id, + title: adr.frontmatter.title, + domain: adr.frontmatter.domain, + source, + }); + } + return items; + } + ); + + const results = await Promise.all(readPromises); + return results.flat(); +} + +// ---------- ID remapping ---------- + +/** + * Build an ID mapping for imported ADRs, assigning new IDs based on domain prefixes. + */ +export function buildIdMap( + adrsToImport: AdrToImport[], + adrsDir: string, + domainPrefixes: Record +): IdMapping[] { + const nextIdByPrefix = new Map(); + const idMap: IdMapping[] = []; + + for (const adr of adrsToImport) { + const prefix = (adr.domain && domainPrefixes[adr.domain]) || "ARCH"; + + if (!nextIdByPrefix.has(prefix)) { + nextIdByPrefix.set(prefix, getNextId(adrsDir, prefix)); + } + + const nextId = nextIdByPrefix.get(prefix)!; + idMap.push({ original: adr.originalId, newId: nextId, title: adr.title }); + + const num = parseInt(nextId.replace(`${prefix}-`, ""), 10) + 1; + nextIdByPrefix.set(prefix, `${prefix}-${String(num).padStart(3, "0")}`); + } + + return idMap; +} + +// ---------- File writing ---------- + +/** + * Write imported ADR files to disk with remapped IDs. + * Returns list of written file paths for rollback on failure. + */ +export async function writeImportedAdrs( + adrsToImport: AdrToImport[], + idMap: IdMapping[], + adrsDir: string +): Promise { + const writtenFiles: string[] = []; + + // Read all source files in parallel first + const readTasks = adrsToImport.map((adr) => Bun.file(adr.sourcePath).text()); + const ruleTasks = adrsToImport.map((adr) => + adr.rulesPath ? Bun.file(adr.rulesPath).text() : Promise.resolve(null) + ); + const [contents, rulesContents] = await Promise.all([ + Promise.all(readTasks), + Promise.all(ruleTasks), + ]); + + try { + for (let i = 0; i < adrsToImport.length; i++) { + const adr = adrsToImport[i]; + const mapping = idMap[i]; + const slug = slugify(mapping.title); + const newFileName = `${mapping.newId}-${slug}.md`; + const destPath = join(adrsDir, newFileName); + + const rewritten = rewriteAdrId( + contents[i], + adr.originalId, + mapping.newId + ); + writeFileSync(destPath, rewritten); + writtenFiles.push(destPath); + + if (rulesContents[i] !== null) { + const rulesFileName = `${mapping.newId}-${slug}.rules.ts`; + const rulesDestPath = join(adrsDir, rulesFileName); + writeFileSync(rulesDestPath, rulesContents[i]!); + writtenFiles.push(rulesDestPath); + } + } + } catch (err) { + // Rollback: delete all written files + for (const file of writtenFiles) { + try { + unlinkSync(file); + } catch { + // Best effort + } + } + throw err; + } + + return writtenFiles; +} + +// ---------- Manifest update ---------- + +/** + * Update the imports manifest with newly imported ADRs. + */ +export function updateImportsManifest( + manifest: ImportsManifest, + adrsToImport: AdrToImport[], + idMap: IdMapping[] +): void { + const sourceGroups = new Map(); + + for (let i = 0; i < adrsToImport.length; i++) { + const adr = adrsToImport[i]; + const mapping = idMap[i]; + const existing = sourceGroups.get(adr.source); + if (existing) { + existing.ids.push(mapping.newId); + } else { + sourceGroups.set(adr.source, { + version: adr.packVersion, + ids: [mapping.newId], + }); + } + } + + for (const [source, group] of sourceGroups) { + manifest.imports.push({ + source, + version: group.version, + importedAt: new Date().toISOString(), + adrIds: group.ids, + }); + } +} + +// ---------- Cleanup ---------- + +export function cleanupTempDirs(dirs: string[]): void { + for (const dir of dirs) { + try { + rmSync(dir, { recursive: true, force: true }); + } catch { + logDebug("Failed to clean up temp dir:", dir); + } + } +} diff --git a/src/helpers/registry.ts b/src/helpers/registry.ts index fb544e0b..c18b4314 100644 --- a/src/helpers/registry.ts +++ b/src/helpers/registry.ts @@ -1,6 +1,6 @@ // SPDX-License-Identifier: Apache-2.0 // Copyright 2026 Archgate -import { existsSync, mkdtempSync, readdirSync } from "node:fs"; +import { existsSync, mkdtempSync, readdirSync, statSync } from "node:fs"; import { tmpdir } from "node:os"; import { join } from "node:path"; @@ -169,13 +169,38 @@ export type ImportTarget = baseDir: string; }; +/** + * List available pack names from a cloned registry repo. + * Looks for directories under `packs/` that contain `archgate-pack.yaml`. + */ +function listAvailablePacks(cloneDir: string): string[] { + const packsDir = join(cloneDir, "packs"); + if (!existsSync(packsDir)) return []; + return readdirSync(packsDir).filter((entry) => { + const entryPath = join(packsDir, entry); + try { + return ( + statSync(entryPath).isDirectory() && + existsSync(join(entryPath, "archgate-pack.yaml")) + ); + } catch { + return false; + } + }); +} + /** * Detect whether the subpath within a cloned repo points to a full pack * (has archgate-pack.yaml) or a single ADR file (.md). + * + * @param sourceKind - The kind of source (official, github-repo, git-url) + * used to tailor error messages. When "official", the error lists available + * packs from the registry. */ export async function detectTarget( cloneDir: string, - subpath: string + subpath: string, + sourceKind?: ResolvedSource["kind"] ): Promise { const fullPath = join(cloneDir, subpath); @@ -214,7 +239,29 @@ export async function detectTarget( }; } + // Build a context-aware error message + const pathExists = existsSync(fullPath); + + if (sourceKind === "official") { + const packName = subpath.replace(/^packs\//u, ""); + const available = listAvailablePacks(cloneDir); + const availableList = + available.length > 0 + ? `\n\nAvailable packs:\n${available.map((p) => ` - packs/${p}`).join("\n")}` + : ""; + throw new Error( + `Pack "${packName}" not found in the official registry.${availableList}` + ); + } + + if (!pathExists) { + throw new Error( + `Path "${subpath}" does not exist in the repository. Verify the path and try again.` + ); + } + throw new Error( - `Cannot detect import target at "${subpath}". Expected archgate-pack.yaml (pack) or a .md file (single ADR).` + `Path "${subpath}" exists but is not a valid import target. ` + + `A pack directory must contain archgate-pack.yaml, or the path must point to a .md file.` ); } diff --git a/tests/helpers/adr-import.test.ts b/tests/helpers/adr-import.test.ts new file mode 100644 index 00000000..a6b913c8 --- /dev/null +++ b/tests/helpers/adr-import.test.ts @@ -0,0 +1,295 @@ +// SPDX-License-Identifier: Apache-2.0 +// Copyright 2026 Archgate +import { afterEach, describe, expect, test } from "bun:test"; +import { + existsSync, + mkdirSync, + mkdtempSync, + rmSync, + writeFileSync, +} from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; + +import type { ImportsManifest } from "../../src/formats/pack"; +import { + buildIdMap, + cleanupTempDirs, + loadImportsManifest, + rewriteAdrId, + saveImportsManifest, + updateImportsManifest, + type AdrToImport, + type IdMapping, +} from "../../src/helpers/adr-import"; + +describe("rewriteAdrId", () => { + test("replaces id in YAML frontmatter", () => { + const content = "---\nid: OLD-001\ntitle: Test\n---\n\n## Context\n"; + const result = rewriteAdrId(content, "OLD-001", "NEW-042"); + expect(result).toContain("id: NEW-042"); + expect(result).not.toContain("OLD-001"); + }); + + test("preserves content outside frontmatter", () => { + const content = + "---\nid: OLD-001\ntitle: Test\n---\n\n## Context\nBody text with OLD-001 reference."; + const result = rewriteAdrId(content, "OLD-001", "NEW-042"); + expect(result).toContain("Body text with OLD-001 reference."); + }); + + test("returns content unchanged when no frontmatter found", () => { + const content = "No frontmatter here."; + const result = rewriteAdrId(content, "OLD-001", "NEW-042"); + expect(result).toBe(content); + }); + + test("handles frontmatter with Windows-style line endings", () => { + const content = "---\r\nid: OLD-001\r\ntitle: Test\r\n---\r\n\r\nBody"; + const result = rewriteAdrId(content, "OLD-001", "NEW-042"); + expect(result).toContain("id: NEW-042"); + }); +}); + +describe("buildIdMap", () => { + let tempDir: string; + + afterEach(() => { + if (tempDir) { + try { + rmSync(tempDir, { recursive: true, force: true }); + } catch { + /* temp dir cleanup */ + } + } + }); + + test("assigns sequential IDs for a single domain prefix", () => { + tempDir = mkdtempSync(join(tmpdir(), "archgate-idmap-test-")); + const adrs: AdrToImport[] = [ + { + sourcePath: "/tmp/a.md", + rulesPath: null, + originalId: "TP-001", + title: "First", + domain: "architecture", + source: "packs/test", + }, + { + sourcePath: "/tmp/b.md", + rulesPath: null, + originalId: "TP-002", + title: "Second", + domain: "architecture", + source: "packs/test", + }, + ]; + + const result = buildIdMap(adrs, tempDir, { architecture: "ARCH" }); + expect(result).toHaveLength(2); + expect(result[0].newId).toBe("ARCH-001"); + expect(result[1].newId).toBe("ARCH-002"); + expect(result[0].original).toBe("TP-001"); + expect(result[1].original).toBe("TP-002"); + }); + + test("skips existing IDs in the adrs directory", () => { + tempDir = mkdtempSync(join(tmpdir(), "archgate-idmap-test-")); + writeFileSync( + join(tempDir, "ARCH-001-existing.md"), + "---\nid: ARCH-001\n---\n" + ); + + const adrs: AdrToImport[] = [ + { + sourcePath: "/tmp/a.md", + rulesPath: null, + originalId: "TP-001", + title: "First", + domain: "architecture", + source: "packs/test", + }, + ]; + + const result = buildIdMap(adrs, tempDir, { architecture: "ARCH" }); + expect(result[0].newId).toBe("ARCH-002"); + }); + + test("falls back to ARCH prefix when domain has no mapping", () => { + tempDir = mkdtempSync(join(tmpdir(), "archgate-idmap-test-")); + const adrs: AdrToImport[] = [ + { + sourcePath: "/tmp/a.md", + rulesPath: null, + originalId: "X-001", + title: "Unknown Domain", + domain: "unknown", + source: "packs/test", + }, + ]; + + const result = buildIdMap(adrs, tempDir, {}); + expect(result[0].newId).toBe("ARCH-001"); + }); + + test("handles multiple domain prefixes independently", () => { + tempDir = mkdtempSync(join(tmpdir(), "archgate-idmap-test-")); + const adrs: AdrToImport[] = [ + { + sourcePath: "/tmp/a.md", + rulesPath: null, + originalId: "A-001", + title: "Arch Rule", + domain: "architecture", + source: "packs/test", + }, + { + sourcePath: "/tmp/b.md", + rulesPath: null, + originalId: "S-001", + title: "Sec Rule", + domain: "security", + source: "packs/test", + }, + { + sourcePath: "/tmp/c.md", + rulesPath: null, + originalId: "A-002", + title: "Arch Rule 2", + domain: "architecture", + source: "packs/test", + }, + ]; + + const result = buildIdMap(adrs, tempDir, { + architecture: "ARCH", + security: "SEC", + }); + expect(result[0].newId).toBe("ARCH-001"); + expect(result[1].newId).toBe("SEC-001"); + expect(result[2].newId).toBe("ARCH-002"); + }); +}); + +describe("updateImportsManifest", () => { + test("adds import entries grouped by source", () => { + const manifest: ImportsManifest = { imports: [] }; + const adrs: AdrToImport[] = [ + { + sourcePath: "/tmp/a.md", + rulesPath: null, + originalId: "TP-001", + title: "First", + source: "packs/test", + packVersion: "1.0.0", + }, + { + sourcePath: "/tmp/b.md", + rulesPath: null, + originalId: "TP-002", + title: "Second", + source: "packs/test", + packVersion: "1.0.0", + }, + ]; + const idMap: IdMapping[] = [ + { original: "TP-001", newId: "ARCH-001", title: "First" }, + { original: "TP-002", newId: "ARCH-002", title: "Second" }, + ]; + + updateImportsManifest(manifest, adrs, idMap); + + expect(manifest.imports).toHaveLength(1); + expect(manifest.imports[0].source).toBe("packs/test"); + expect(manifest.imports[0].version).toBe("1.0.0"); + expect(manifest.imports[0].adrIds).toEqual(["ARCH-001", "ARCH-002"]); + expect(manifest.imports[0].importedAt).toBeTruthy(); + }); + + test("preserves existing manifest entries", () => { + const manifest: ImportsManifest = { + imports: [ + { + source: "packs/existing", + version: "0.5.0", + importedAt: "2026-01-01T00:00:00.000Z", + adrIds: ["GEN-001"], + }, + ], + }; + const adrs: AdrToImport[] = [ + { + sourcePath: "/tmp/a.md", + rulesPath: null, + originalId: "TP-001", + title: "New", + source: "packs/new", + packVersion: "2.0.0", + }, + ]; + const idMap: IdMapping[] = [ + { original: "TP-001", newId: "ARCH-005", title: "New" }, + ]; + + updateImportsManifest(manifest, adrs, idMap); + + expect(manifest.imports).toHaveLength(2); + expect(manifest.imports[0].source).toBe("packs/existing"); + expect(manifest.imports[1].source).toBe("packs/new"); + }); +}); + +describe("loadImportsManifest / saveImportsManifest", () => { + let tempDir: string; + + afterEach(() => { + if (tempDir) { + try { + rmSync(tempDir, { recursive: true, force: true }); + } catch { + /* temp dir cleanup */ + } + } + }); + + test("returns empty manifest when imports.json does not exist", async () => { + tempDir = mkdtempSync(join(tmpdir(), "archgate-manifest-test-")); + mkdirSync(join(tempDir, ".archgate"), { recursive: true }); + const manifest = await loadImportsManifest(tempDir); + expect(manifest.imports).toEqual([]); + }); + + test("round-trips a manifest through save and load", async () => { + tempDir = mkdtempSync(join(tmpdir(), "archgate-manifest-test-")); + mkdirSync(join(tempDir, ".archgate"), { recursive: true }); + const original: ImportsManifest = { + imports: [ + { + source: "packs/test", + version: "1.0.0", + importedAt: "2026-06-13T00:00:00.000Z", + adrIds: ["ARCH-001"], + }, + ], + }; + saveImportsManifest(tempDir, original); + const loaded = await loadImportsManifest(tempDir); + expect(loaded).toEqual(original); + }); +}); + +describe("cleanupTempDirs", () => { + test("removes existing directories", () => { + const dir = mkdtempSync(join(tmpdir(), "archgate-cleanup-test-")); + writeFileSync(join(dir, "file.txt"), "test"); + expect(existsSync(dir)).toBe(true); + cleanupTempDirs([dir]); + expect(existsSync(dir)).toBe(false); + }); + + test("does not throw for non-existent directories", () => { + expect(() => + cleanupTempDirs(["/tmp/nonexistent-archgate-dir"]) + ).not.toThrow(); + }); +}); diff --git a/tests/helpers/registry.test.ts b/tests/helpers/registry.test.ts index 6515d7a0..6c91f544 100644 --- a/tests/helpers/registry.test.ts +++ b/tests/helpers/registry.test.ts @@ -262,20 +262,48 @@ describe("detectTarget", () => { } }); - test("throws when subpath is neither a pack nor an ADR", async () => { + test("throws descriptive error when directory exists but is not a valid target", async () => { tempDir = mkdtempSync(join(tmpdir(), "archgate-registry-test-")); mkdirSync(join(tempDir, "empty-dir")); await expect(detectTarget(tempDir, "empty-dir")).rejects.toThrow( - /Cannot detect import target/u + /exists but is not a valid import target/u ); }); - test("throws when subpath does not exist", async () => { + test("throws descriptive error when subpath does not exist", async () => { tempDir = mkdtempSync(join(tmpdir(), "archgate-registry-test-")); await expect(detectTarget(tempDir, "nonexistent")).rejects.toThrow( - /Cannot detect import target/u + /does not exist in the repository/u ); }); + + test("lists available packs when official registry pack is not found", async () => { + tempDir = mkdtempSync(join(tmpdir(), "archgate-registry-test-")); + // Simulate an official registry clone with one valid pack + const packsDir = join(tempDir, "packs"); + const validPack = join(packsDir, "typescript-strict"); + mkdirSync(validPack, { recursive: true }); + writeFileSync( + join(validPack, "archgate-pack.yaml"), + [ + "name: typescript-strict", + "version: 1.0.0", + "description: Strict TS", + "maintainers:", + " - github: testuser", + ].join("\n") + ); + + try { + await detectTarget(tempDir, "packs/nonexistent", "official"); + expect.unreachable("Should have thrown"); + } catch (err) { + expect(err).toBeInstanceOf(Error); + const message = (err as Error).message; + expect(message).toContain("not found in the official registry"); + expect(message).toContain("packs/typescript-strict"); + } + }); }); From 5c090484789b62b6763246525d023e784b8a25b7 Mon Sep 17 00:00:00 2001 From: Rhuan Barreto Date: Sat, 13 Jun 2026 22:09:18 +0200 Subject: [PATCH 2/5] fix(import): address review feedback on error handling - Use safeParse() instead of .parse() in loadImportsManifest for user-friendly validation errors per coding guidelines - Guard official-source "pack not found" branch with !pathExists so existing-but-invalid paths still get the correct diagnostic - Move tempDirs cleanup to a finally block so temp directories are always removed, even on error paths Signed-off-by: Rhuan Barreto --- src/commands/adr/import.ts | 14 +++++++------- src/helpers/adr-import.ts | 8 +++++++- src/helpers/registry.ts | 2 +- 3 files changed, 15 insertions(+), 9 deletions(-) diff --git a/src/commands/adr/import.ts b/src/commands/adr/import.ts index 4ef275af..61f1f63e 100644 --- a/src/commands/adr/import.ts +++ b/src/commands/adr/import.ts @@ -38,6 +38,7 @@ export function registerAdrImportCommand(adr: Command) { .option("--dry-run", "Preview changes without writing", false) .option("--list", "List previously imported ADRs", false) .action(async (sources, opts) => { + let tempDirs: string[] = []; try { const projectRoot = findProjectRoot(); if (!projectRoot) { @@ -72,7 +73,9 @@ export function registerAdrImportCommand(adr: Command) { // ---------- Resolve & clone ---------- - const { resolved, tempDirs } = await resolveAndCloneSources(sources); + const cloned = await resolveAndCloneSources(sources); + const { resolved } = cloned; + tempDirs = cloned.tempDirs; // ---------- Collect ADR files ---------- @@ -80,7 +83,6 @@ export function registerAdrImportCommand(adr: Command) { if (adrsToImport.length === 0) { console.log("No ADRs found to import."); - cleanupTempDirs(tempDirs); return; } @@ -132,7 +134,6 @@ export function registerAdrImportCommand(adr: Command) { } else { console.log("Dry run — no files written."); } - cleanupTempDirs(tempDirs); return; } @@ -152,7 +153,6 @@ export function registerAdrImportCommand(adr: Command) { ); if (!confirm) { console.log("Import cancelled."); - cleanupTempDirs(tempDirs); return; } } @@ -171,9 +171,7 @@ export function registerAdrImportCommand(adr: Command) { await ensureRulesShim(projectRoot, paths.adrsDir); - // ---------- Cleanup & summary ---------- - - cleanupTempDirs(tempDirs); + // ---------- Summary ---------- if (useJson) { console.log( @@ -200,6 +198,8 @@ export function registerAdrImportCommand(adr: Command) { if (err instanceof Error && err.name === "ExitPromptError") throw err; logError(err instanceof Error ? err.message : String(err)); await exitWith(1); + } finally { + if (tempDirs.length > 0) cleanupTempDirs(tempDirs); } }); } diff --git a/src/helpers/adr-import.ts b/src/helpers/adr-import.ts index 7ae0bede..90653c3b 100644 --- a/src/helpers/adr-import.ts +++ b/src/helpers/adr-import.ts @@ -46,7 +46,13 @@ export async function loadImportsManifest( const importsPath = join(projectRoot, ".archgate", "imports.json"); if (!existsSync(importsPath)) return { imports: [] }; const raw = await Bun.file(importsPath).json(); - return ImportsManifestSchema.parse(raw); + const result = ImportsManifestSchema.safeParse(raw); + if (!result.success) { + throw new Error( + `Invalid imports manifest at ${importsPath}: ${result.error.issues.map((i) => i.message).join(", ")}` + ); + } + return result.data; } export function saveImportsManifest( diff --git a/src/helpers/registry.ts b/src/helpers/registry.ts index c18b4314..2f4290a2 100644 --- a/src/helpers/registry.ts +++ b/src/helpers/registry.ts @@ -242,7 +242,7 @@ export async function detectTarget( // Build a context-aware error message const pathExists = existsSync(fullPath); - if (sourceKind === "official") { + if (!pathExists && sourceKind === "official") { const packName = subpath.replace(/^packs\//u, ""); const available = listAvailablePacks(cloneDir); const availableList = From 556d9511209256da12e023ad3a1bc45bd0cf764c Mon Sep 17 00:00:00 2001 From: Rhuan Barreto Date: Sat, 13 Jun 2026 22:19:42 +0200 Subject: [PATCH 3/5] fix(import): clean up partial temp dirs when resolveAndCloneSources fails If shallowClone succeeds for source #1 but a later source fails (clone error, detectTarget rejection), the promise rejects before the caller can capture tempDirs, leaking clone #1 on disk. Wrap the clone loop in try/catch so resolveAndCloneSources cleans up any temp dirs it already created before re-throwing. Signed-off-by: Rhuan Barreto --- src/helpers/adr-import.ts | 29 +++++++++++++++++------------ 1 file changed, 17 insertions(+), 12 deletions(-) diff --git a/src/helpers/adr-import.ts b/src/helpers/adr-import.ts index 90653c3b..06153666 100644 --- a/src/helpers/adr-import.ts +++ b/src/helpers/adr-import.ts @@ -94,21 +94,26 @@ export async function resolveAndCloneSources( const resolved: ResolvedImport[] = []; const cloneCache = new Map(); - for (const source of sources) { - const res = resolveSource(source); - logDebug("Resolved source:", JSON.stringify(res)); + try { + for (const source of sources) { + const res = resolveSource(source); + logDebug("Resolved source:", JSON.stringify(res)); - const cacheKey = `${res.repoUrl}#${res.ref ?? ""}`; - let cloneDir = cloneCache.get(cacheKey); + const cacheKey = `${res.repoUrl}#${res.ref ?? ""}`; + let cloneDir = cloneCache.get(cacheKey); - if (!cloneDir) { - cloneDir = await shallowClone(res.repoUrl, res.ref); // eslint-disable-line no-await-in-loop -- sequential by design (dedup) - cloneCache.set(cacheKey, cloneDir); - tempDirs.push(cloneDir); - } + if (!cloneDir) { + cloneDir = await shallowClone(res.repoUrl, res.ref); // eslint-disable-line no-await-in-loop -- sequential by design (dedup) + cloneCache.set(cacheKey, cloneDir); + tempDirs.push(cloneDir); + } - const target = await detectTarget(cloneDir, res.subpath, res.kind); // eslint-disable-line no-await-in-loop -- depends on prior clone - resolved.push({ source, target, cloneDir }); + const target = await detectTarget(cloneDir, res.subpath, res.kind); // eslint-disable-line no-await-in-loop -- depends on prior clone + resolved.push({ source, target, cloneDir }); + } + } catch (err) { + cleanupTempDirs(tempDirs); + throw err; } return { resolved, tempDirs }; From b716a005ae3cf72f5bd38f169e59a4c6121ffdec Mon Sep 17 00:00:00 2001 From: Rhuan Barreto Date: Sat, 13 Jun 2026 22:27:29 +0200 Subject: [PATCH 4/5] fix(import): clean up temp dir when shallowClone git command fails shallowClone creates a temp directory before running git clone. On clone failure it threw without removing the directory, leaking archgate-import-* dirs in the system temp area. Now rmSync runs before re-throwing. Signed-off-by: Rhuan Barreto --- src/helpers/registry.ts | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/src/helpers/registry.ts b/src/helpers/registry.ts index 2f4290a2..edd627f8 100644 --- a/src/helpers/registry.ts +++ b/src/helpers/registry.ts @@ -1,6 +1,12 @@ // SPDX-License-Identifier: Apache-2.0 // Copyright 2026 Archgate -import { existsSync, mkdtempSync, readdirSync, statSync } from "node:fs"; +import { + existsSync, + mkdtempSync, + readdirSync, + rmSync, + statSync, +} from "node:fs"; import { tmpdir } from "node:os"; import { join } from "node:path"; @@ -144,6 +150,7 @@ export async function shallowClone( const result = await run(args); if (result.exitCode !== 0) { + rmSync(tempDir, { recursive: true, force: true }); throw new Error( `Failed to clone ${repoUrl}${ref ? ` (ref: ${ref})` : ""}:\n${result.stderr.trim()}` ); From 3ca30afb0cb84f96fed2e4734eaabbb38bfbe522 Mon Sep 17 00:00:00 2001 From: Rhuan Barreto Date: Sat, 13 Jun 2026 22:35:44 +0200 Subject: [PATCH 5/5] fix(import): handle spawn failures and block path traversal in registry - Wrap run(args) in shallowClone with try/catch so the temp dir is removed when the spawn itself throws, not only on non-zero exit - Validate that subpath in detectTarget resolves within cloneDir, rejecting inputs containing ../ that would escape the clone root Signed-off-by: Rhuan Barreto --- src/helpers/registry.ts | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-) diff --git a/src/helpers/registry.ts b/src/helpers/registry.ts index edd627f8..06b37c10 100644 --- a/src/helpers/registry.ts +++ b/src/helpers/registry.ts @@ -8,7 +8,7 @@ import { statSync, } from "node:fs"; import { tmpdir } from "node:os"; -import { join } from "node:path"; +import { isAbsolute, join, relative, resolve } from "node:path"; import type { PackMetadata } from "../formats/pack"; import { parsePackMetadata } from "../formats/pack"; @@ -147,7 +147,13 @@ export async function shallowClone( args.push(repoUrl, tempDir); logDebug("Cloning:", args.join(" ")); - const result = await run(args); + let result: { exitCode: number; stdout: string; stderr: string }; + try { + result = await run(args); + } catch (err) { + rmSync(tempDir, { recursive: true, force: true }); + throw err; + } if (result.exitCode !== 0) { rmSync(tempDir, { recursive: true, force: true }); @@ -209,7 +215,11 @@ export async function detectTarget( subpath: string, sourceKind?: ResolvedSource["kind"] ): Promise { - const fullPath = join(cloneDir, subpath); + const fullPath = resolve(cloneDir, subpath); + const rel = relative(cloneDir, fullPath); + if (rel.startsWith("..") || isAbsolute(rel)) { + throw new Error(`Path "${subpath}" escapes the repository root.`); + } // Check for a pack (directory with archgate-pack.yaml) const packYaml = join(fullPath, "archgate-pack.yaml");