fix(import): improve error messages and extract business logic#413
Open
rhuanbarreto wants to merge 5 commits into
Open
fix(import): improve error messages and extract business logic#413rhuanbarreto wants to merge 5 commits into
rhuanbarreto wants to merge 5 commits into
Conversation
…ommand 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 <rhuan@barreto.work>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughRefactors the ADR import command to delegate cloning, ADR collection, ID remapping, file writes, and imports manifest updates to a new ChangesADR Import Helper Extraction
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Deploying archgate-cli with
|
| Latest commit: |
3ca30af
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://eaafa854.archgate-cli.pages.dev |
| Branch Preview URL: | https://fix-import-error-messages-an.archgate-cli.pages.dev |
Contributor
Code Coverage
Full HTML report available in workflow artifacts. Per-directory breakdown
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/commands/adr/import.ts (1)
75-83:⚠️ Potential issue | 🟠 Major | ⚡ Quick winTemp directories are not guaranteed to be cleaned on exception paths.
cleanupTempDirs(tempDirs)is called on success and selected early returns, but not when errors occur after cloning (e.g., duringcollectAdrsToImport,writeImportedAdrs, or manifest save). The catch block exits without cleanup.Suggested fix
- .action(async (sources, opts) => { - try { + .action(async (sources, opts) => { + let tempDirs: string[] = []; + try { const projectRoot = findProjectRoot(); if (!projectRoot) { logError("No .archgate/ directory found. Run `archgate init` first."); await exitWith(1); return; } @@ - const { resolved, tempDirs } = await resolveAndCloneSources(sources); + const cloned = await resolveAndCloneSources(sources); + const { resolved } = cloned; + tempDirs = cloned.tempDirs; @@ - cleanupTempDirs(tempDirs); return; } @@ - cleanupTempDirs(tempDirs); return; } @@ - cleanupTempDirs(tempDirs); return; } } @@ - cleanupTempDirs(tempDirs); - if (useJson) { @@ } catch (err) { 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); } });Also applies to: 135-136, 155-156, 176-176
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/commands/adr/import.ts` around lines 75 - 83, The temp directories returned by resolveAndCloneSources are not cleaned on error paths; wrap the work that uses tempDirs (the calls to collectAdrsToImport, writeImportedAdrs, manifest save, and any early returns) in a try/finally so cleanupTempDirs(tempDirs) always runs: call resolveAndCloneSources(sources) to get { resolved, tempDirs }, then do the processing inside try { ... } and place cleanupTempDirs(tempDirs) in finally { ... }; ensure any early returns inside the try still allow the finally to execute and remove any duplicate cleanup calls elsewhere (remove the existing direct calls to cleanupTempDirs(tempDirs) where they become redundant).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/helpers/adr-import.ts`:
- Around line 43-50: Replace the direct Zod .parse call in loadImportsManifest
with .safeParse and handle the result: call
ImportsManifestSchema.safeParse(raw), if the result is successful return
result.data, otherwise throw a new Error with a clear, user-friendly message
that includes the validation errors (derived from the safeParse result) so
callers get readable feedback instead of Zod's internal error format.
In `@src/helpers/registry.ts`:
- Around line 245-261: The current logic checks sourceKind === "official" before
verifying pathExists, which causes the code in the official branch to always
throw the "Pack not found" error even when the path exists but points to an
invalid target; fix this by ensuring path existence is validated first (or by
adding pathExists to the official-branch condition). Concretely, update the code
around the checks for sourceKind, packName, listAvailablePacks, pathExists, and
subpath so that you either move the if (!pathExists) block to run before the if
(sourceKind === "official") block, or change the official branch to only throw
the "Pack not found" error when sourceKind === "official" && !pathExists
(leaving the existing invalid-target diagnostic to run when pathExists is true).
---
Outside diff comments:
In `@src/commands/adr/import.ts`:
- Around line 75-83: The temp directories returned by resolveAndCloneSources are
not cleaned on error paths; wrap the work that uses tempDirs (the calls to
collectAdrsToImport, writeImportedAdrs, manifest save, and any early returns) in
a try/finally so cleanupTempDirs(tempDirs) always runs: call
resolveAndCloneSources(sources) to get { resolved, tempDirs }, then do the
processing inside try { ... } and place cleanupTempDirs(tempDirs) in finally {
... }; ensure any early returns inside the try still allow the finally to
execute and remove any duplicate cleanup calls elsewhere (remove the existing
direct calls to cleanupTempDirs(tempDirs) where they become redundant).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 0176c245-db16-4517-a1aa-cf8a2b47790b
📒 Files selected for processing (5)
src/commands/adr/import.tssrc/helpers/adr-import.tssrc/helpers/registry.tstests/helpers/adr-import.test.tstests/helpers/registry.test.ts
- 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 <rhuan@barreto.work>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/helpers/registry.ts (1)
245-255:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRestrict the official-registry “pack not found” branch to
packs/<name>.This still catches any missing official subpath, so an input like
packs/existing-pack/adrs/missingreportsPack "existing-pack/adrs/missing" not found...even when the pack exists. Limit this branch to top-level pack paths and let deeper misses fall through to the generic “path does not exist” diagnostic.Suggested fix
- if (!pathExists && sourceKind === "official") { + if ( + !pathExists && + sourceKind === "official" && + /^packs\/[^/]+$/u.test(subpath) + ) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/helpers/registry.ts` around lines 245 - 255, The branch that throws the “Pack ... not found in the official registry” error should only run for top-level pack paths; change the condition around pathExists/sourceKind/subpath so it only triggers when sourceKind === "official" AND subpath matches a top-level pack pattern (e.g., /^packs\/[^/]+$/) before computing packName and calling listAvailablePacks(cloneDir); leave deeper paths (like packs/<pack>/...) to fall through to the generic missing-path handling so that missing inner files/dirs are reported by the existing fallback. Use the existing identifiers subpath, pathExists, sourceKind, packName, listAvailablePacks, and cloneDir to locate and update the code.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/commands/adr/import.ts`:
- Around line 76-78: The finally block leaks temp dirs because tempDirs is only
assigned after await resolveAndCloneSources(sources); fix by ensuring partial
temp dirs are tracked before the await resolves: either (A) modify
resolveAndCloneSources to push each created temp dir into a shared array
(tempDirs) as soon as shallowClone creates it (ensure shallowClone calls a
registration helper when it creates a temp dir), and have resolveAndCloneSources
reject while still returning/maintaining that array so the outer finally can
clean it up; or (B) move responsibility for cleanup into
resolveAndCloneSources/shallowClone so they always cleanup their own temp dirs
on error and the outer finally becomes a best-effort fallback; reference the
functions resolveAndCloneSources and shallowClone and the tempDirs variable when
applying the change.
---
Duplicate comments:
In `@src/helpers/registry.ts`:
- Around line 245-255: The branch that throws the “Pack ... not found in the
official registry” error should only run for top-level pack paths; change the
condition around pathExists/sourceKind/subpath so it only triggers when
sourceKind === "official" AND subpath matches a top-level pack pattern (e.g.,
/^packs\/[^/]+$/) before computing packName and calling
listAvailablePacks(cloneDir); leave deeper paths (like packs/<pack>/...) to fall
through to the generic missing-path handling so that missing inner files/dirs
are reported by the existing fallback. Use the existing identifiers subpath,
pathExists, sourceKind, packName, listAvailablePacks, and cloneDir to locate and
update the code.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: bd935e85-6ef9-4a71-b0ac-6f6f96ae94dd
📒 Files selected for processing (3)
src/commands/adr/import.tssrc/helpers/adr-import.tssrc/helpers/registry.ts
Comment on lines
+76
to
+78
| const cloned = await resolveAndCloneSources(sources); | ||
| const { resolved } = cloned; | ||
| tempDirs = cloned.tempDirs; |
There was a problem hiding this comment.
finally still misses temp dirs created before resolveAndCloneSources() rejects.
tempDirs is only populated after resolveAndCloneSources() returns. If that helper clones one repo and then fails on a later source (or if shallowClone() fails after creating its temp directory), the promise rejects before this assignment runs, so the finally block cleans nothing and those temp dirs leak on disk. The helper layer needs to own cleanup on failure, or otherwise expose partial tempDirs before the await completes.
Also applies to: 201-202
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/commands/adr/import.ts` around lines 76 - 78, The finally block leaks
temp dirs because tempDirs is only assigned after await
resolveAndCloneSources(sources); fix by ensuring partial temp dirs are tracked
before the await resolves: either (A) modify resolveAndCloneSources to push each
created temp dir into a shared array (tempDirs) as soon as shallowClone creates
it (ensure shallowClone calls a registration helper when it creates a temp dir),
and have resolveAndCloneSources reject while still returning/maintaining that
array so the outer finally can clean it up; or (B) move responsibility for
cleanup into resolveAndCloneSources/shallowClone so they always cleanup their
own temp dirs on error and the outer finally becomes a best-effort fallback;
reference the functions resolveAndCloneSources and shallowClone and the tempDirs
variable when applying the change.
…ails 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 <rhuan@barreto.work>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/helpers/adr-import.ts (1)
133-145:⚠️ Potential issue | 🟠 MajorSort pack ADRs before building the import list to make ID assignment deterministic.
src/helpers/registry.tsbuildstarget.adrFiles/target.rulesFilesfromreaddirSync(adrsDir)without sorting.src/helpers/adr-import.tsthen consumestarget.adrFilesin-order to constructadrsToImport, andbuildIdMap()assignsnewIdvalues sequentially over that order—so pack ID mappings (andimports.jsonentries) can vary across filesystems/runs.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/helpers/adr-import.ts` around lines 133 - 145, The import order for pack ADRs is non-deterministic because target.adrFiles is consumed as-is when building adrsToImport (the block starting with "if (target.kind === \"pack\")" that calls Bun.file(...).text(), parseAdr(...), basename(...), and items.push(...)), which causes buildIdMap() to assign newId values in filesystem order; fix this by deterministically sorting the ADR list before processing (e.g., const sortedAdrFiles = [...target.adrFiles].sort() and use sortedAdrFiles for the Promise.all and subsequent loop) and likewise sort or otherwise deterministically handle target.rulesFiles if you rely on their order—this ensures the constructed items/adrsToImport and resulting ID map are stable across runs.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/helpers/adr-import.ts`:
- Around line 105-116: The clone temp directory can be created inside
shallowClone but not returned on failure, so failed clones leak temp dirs;
inside the shallowClone function wrap the git clone step in a try/catch and on
any error synchronously/awaitably remove the temp directory you created (e.g.,
fs.rm/recursive) before rethrowing the error so callers (like the try/catch in
the import flow that only tracks returned cloneDir values) don't need to change;
keep the existing cleanupTempDirs usage as is.
---
Outside diff comments:
In `@src/helpers/adr-import.ts`:
- Around line 133-145: The import order for pack ADRs is non-deterministic
because target.adrFiles is consumed as-is when building adrsToImport (the block
starting with "if (target.kind === \"pack\")" that calls Bun.file(...).text(),
parseAdr(...), basename(...), and items.push(...)), which causes buildIdMap() to
assign newId values in filesystem order; fix this by deterministically sorting
the ADR list before processing (e.g., const sortedAdrFiles =
[...target.adrFiles].sort() and use sortedAdrFiles for the Promise.all and
subsequent loop) and likewise sort or otherwise deterministically handle
target.rulesFiles if you rely on their order—this ensures the constructed
items/adrsToImport and resulting ID map are stable across runs.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 6fd69185-6cdd-4954-b788-6a004b8c2e2d
📒 Files selected for processing (1)
src/helpers/adr-import.ts
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 <rhuan@barreto.work>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/helpers/registry.ts (2)
149-154:⚠️ Potential issue | 🟠 Major | ⚡ Quick winClean up temp dirs when
run(args)throws, not only on non-zero exit.At Line 150, if
run(args)throws (spawn/stream failure), cleanup at Line 153 is skipped and the temp directory leaks. Wraprun(args)intry/catchand removetempDirbefore rethrowing.Suggested patch
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 }); throw new Error(🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/helpers/registry.ts` around lines 149 - 154, The code currently calls run(args) without catching exceptions so rmSync(tempDir, { recursive: true, force: true }) only runs when result.exitCode !== 0; wrap the run(args) call in a try/catch (or try/finally) around the block that includes logDebug and run so that on any thrown error you call rmSync(tempDir, { recursive: true, force: true }) before rethrowing the original error; keep the existing non-zero-exit handling (checking result.exitCode) but remove duplicate cleanup by centralizing tempDir removal in the catch or finally so temp dirs are always removed whether run throws or returns a non-zero exit code.
212-213:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winConstrain
subpathto the clone root to block path traversal.
subpathis joined directly at Line 212, so inputs containing../can escapecloneDirand make Line 217/Line 239 read arbitrary local files. Please normalize and enforce that the resolved target stays withincloneDirbefore any filesystem access.Suggested patch
-import { join } from "node:path"; +import { isAbsolute, join, relative, resolve } from "node:path"; ... export async function detectTarget( cloneDir: string, subpath: string, sourceKind?: ResolvedSource["kind"] ): Promise<ImportTarget> { - 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.`); + }Also applies to: 217-218, 238-240
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/helpers/registry.ts` around lines 212 - 213, The code currently joins user-controlled subpath with cloneDir into fullPath (variable fullPath) and then reads files, allowing path traversal; fix by resolving and validating the target stays inside cloneDir before any FS access: compute const resolved = path.resolve(cloneDir, subpath) (or use fs.realpathSync on cloneDir and resolved for symlink safety), then verify that resolved === path.resolve(cloneDir) or resolved.startsWith(path.resolve(cloneDir) + path.sep); if the check fails, throw or return an error and do not perform the read; apply this same validation wherever fullPath/target is constructed from subpath (the places using cloneDir, subpath, and fullPath) so all reads are gated by this containment check.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/helpers/registry.ts`:
- Around line 149-154: The code currently calls run(args) without catching
exceptions so rmSync(tempDir, { recursive: true, force: true }) only runs when
result.exitCode !== 0; wrap the run(args) call in a try/catch (or try/finally)
around the block that includes logDebug and run so that on any thrown error you
call rmSync(tempDir, { recursive: true, force: true }) before rethrowing the
original error; keep the existing non-zero-exit handling (checking
result.exitCode) but remove duplicate cleanup by centralizing tempDir removal in
the catch or finally so temp dirs are always removed whether run throws or
returns a non-zero exit code.
- Around line 212-213: The code currently joins user-controlled subpath with
cloneDir into fullPath (variable fullPath) and then reads files, allowing path
traversal; fix by resolving and validating the target stays inside cloneDir
before any FS access: compute const resolved = path.resolve(cloneDir, subpath)
(or use fs.realpathSync on cloneDir and resolved for symlink safety), then
verify that resolved === path.resolve(cloneDir) or
resolved.startsWith(path.resolve(cloneDir) + path.sep); if the check fails,
throw or return an error and do not perform the read; apply this same validation
wherever fullPath/target is constructed from subpath (the places using cloneDir,
subpath, and fullPath) so all reads are gated by this containment check.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: fc342038-31de-42ba-9e1e-d6efd2a5d300
📒 Files selected for processing (1)
src/helpers/registry.ts
- 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 <rhuan@barreto.work>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
src/commands/adr/import.tsintosrc/helpers/adr-import.ts(ARCH-001 compliance — command files should be thin I/O only)findProjectRoot()inside the try-catch error boundary (ARCH-012 compliance)Test plan
bun run validatepasses (1300 tests, 39/39 ADR rules, lint/typecheck/format/knip/build clean)rewriteAdrId,buildIdMap,updateImportsManifest,loadImportsManifest/saveImportsManifest,cleanupTempDirs(14 new tests)detectTargettests for new error message patternsSummary by CodeRabbit
New Features
Refactor
Bug Fixes
Tests