Skip to content

🐛 fix(cli): prefix artefact paths with cwd-relative target path in diff output#204

Merged
vincent-psarga merged 15 commits intomainfrom
bug-diff-target-cli
Mar 13, 2026
Merged

🐛 fix(cli): prefix artefact paths with cwd-relative target path in diff output#204
vincent-psarga merged 15 commits intomainfrom
bug-diff-target-cli

Conversation

@cteyton
Copy link
Copy Markdown
Contributor

@cteyton cteyton commented Mar 12, 2026

Explanation

In a monorepo with multiple Packmind targets (each with their own packmind.json), the diff command was showing artefact file paths relative to their target directory (e.g. .claude/rules/packmind/standard.md), making it ambiguous which project the file belongs to.

This PR fixes three related issues:

  1. Artefact paths are now always prefixed with the target's path relative to the working directory (e.g. src/frontend/.claude/rules/packmind/standard.md)
  2. The prefix is applied even in single-target scenarios when the target is a subdirectory (e.g. --path src with target at src/frontend)
  3. Blank packmind-cli lines that appeared before each "Target:" header (caused by a \n prefix in logInfoConsole) are removed

Type of Change

  • Bug fix

Affected Components

  • Domain packages affected: none
  • Frontend / Backend / Both: CLI only
  • Breaking changes (if any): none — display output changes only

Testing

  • Unit tests added/updated
  • Integration tests added/updated
  • Manual testing completed
  • Test coverage maintained or improved

Test Details:

  • Added tests for multi-target path prefixing (frontend + backend targets)
  • Added tests verifying no spurious prefix when target IS the cwd
  • All 1852 existing tests pass

TODO List

  • CHANGELOG Updated
  • Documentation Updated

Reviewer Notes

The core fix is in diffArtefactsHandler.ts: targetRelativePath is now computed via nodePath.relative(cwd, targetDir) instead of slicing from gitRoot. The displayPathMap always applies this prefix — when the target is the cwd, nodePath.relative returns '' so no prefix is added.

cteyton and others added 4 commits March 12, 2026 13:15
…get diff output

When running `packmind-cli diff` in a monorepo with multiple targets,
file paths are now prefixed with the target's relative path to make
their location unambiguous.

- Build a displayPathMap after collecting targetResults
- Pass displayPathMap to displayDiffs and use it for path display
- Single-target case is unchanged (no prefix added)
- Add tests for multi-target path prefixing and single-target no-prefix

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…n multi-target diff

The \n prefix in logInfoConsole caused an empty "packmind-cli" line
to appear before each "Target:" line. Replace with a plain log('')
for the blank line separator.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…t root

Artefact file paths in multi-target diff output were prefixed with the
path relative to the git root, making them incorrect when the command
is run from a subdirectory. Now uses nodePath.relative(cwd, targetDir)
so paths are always relative to where the command is invoked.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…f target count

The prefix was only applied in multi-target scenarios, leaving single-target
cases (e.g. --path src with one target at src/frontend) showing paths relative
to the target directory instead of cwd. Now the prefix is always applied;
when the target IS the cwd, targetRelativePath is empty and no prefix is added.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Comment thread apps/cli/src/infra/commands/diffArtefactsHandler.spec.ts Outdated
@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented Mar 12, 2026

Greptile Summary

This PR fixes artefact path display in the diff command by introducing multi-target discovery (configExists + findDescendantConfigs) and building a displayPathMap that prefixes each artefact's file path with nodePath.relative(cwd, targetDir), so paths are always shown relative to the working directory rather than the target's own directory. It also threads findNearestConfigDir into diffAddHandler and diffRemoveHandler so those commands correctly scope their config/git lookups to the nearest packmind.json rather than blindly using cwd.

Key changes:

  • diffArtefactsHandler: replaces single-target readFullConfig(cwd) with a loop over discovered targets; adds --path/-p flag to scope the search directory; removes the error dep in favour of logErrorConsole throughout
  • findNearestConfigDir: new shared utility for walking up the directory tree to the nearest packmind.json
  • diffAddHandler / diffRemoveHandler: both now infer targetDir via findNearestConfigDir from the submitted file's location, fixing config and git-root lookups in monorepo subdirectory scenarios
  • Tests: multi-target describe blocks added with realistic mock data; single-target tests moved to their own describe block

One gap remains: when --path is validated with fs.stat, the code only checks that the path exists — it never calls .isDirectory(). Passing a file path via --path silently passes the validation and produces confusing downstream errors instead of an actionable message. The test mock already sets up isDirectory: () => true, suggesting this check was intended.

Confidence Score: 4/5

  • Safe to merge — the core path-prefixing fix is correct and well-tested; the one logic gap (no directory check for --path) is a UX issue, not a data-corruption risk.
  • The primary fix (multi-target artefact path display) is sound, correctly uses nodePath.relative to produce an empty string when target == cwd, and existing tests have been updated to match realistic behaviour. The add/remove refactors are straightforward and correctly infer target directories. The only actionable logic issue is that fs.stat doesn't verify the --path value is a directory, which could cause confusing error messages in a narrow edge case but poses no correctness or data risk.
  • Pay close attention to apps/cli/src/infra/commands/diffArtefactsHandler.ts (the --path directory validation gap around line 457) and apps/cli/CHANGELOG.MD (entry mis-categorised under ## Added).

Important Files Changed

Filename Overview
apps/cli/src/infra/commands/diffArtefactsHandler.ts Core fix: replaces single-target cwd-based logic with multi-target discovery via configExists/findDescendantConfigs; introduces displayPathMap to prefix artefact paths with nodePath.relative(cwd, targetDir). One gap: fs.stat validates path existence but not that it's a directory, so a file path passed via --path causes confusing downstream errors instead of a clear diagnostic.
apps/cli/src/infra/commands/diffArtefactsHandler.spec.ts Tests updated significantly: removed mockError, added multi-target describe blocks with correct mock data (realistic .packmind/commands/cmd.md paths without double-prefix). Single-target describe block correctly placed at its own level. The --path scopes the search test only exercises configExists routing, not path-prefixing of actual diffs.
apps/cli/src/infra/commands/DiffCommand.ts Adds --path/-p option; for add/remove subcommands it prepends the path to the file argument, for the main diff command it passes path through to diffArtefactsHandler. Removes error: console.error from handler dependencies. Clean and consistent.
apps/cli/src/application/utils/findNearestConfigDir.ts New utility that walks up the directory tree using configExists to find the nearest packmind.json. Correctly terminates at filesystem root. Used in diffAddHandler and diffRemoveHandler.
apps/cli/src/infra/commands/diffAddHandler.ts Now uses findNearestConfigDir instead of reading config from cwd directly, and uses nodePath.relative(gitRoot, targetDir) for relativePath. Correctly infers fileDir with special handling for SKILL.md. Logic is sound.
apps/cli/src/infra/commands/diffRemoveHandler.ts Parallel refactor to diffAddHandler: uses findNearestConfigDir and reads config/git root from targetDir. computeFilePathRelativeToTarget updated to use path.relative instead of string slice, avoiding edge cases with paths that don't start with gitRoot string.
apps/cli/CHANGELOG.MD Entry for this bug fix was placed under ## Added instead of the existing empty ## Fixed section in the [Unreleased] block.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[diffArtefactsHandler called] --> B{--path provided?}
    B -- Yes --> C[fs.stat validate path exists]
    C -- fails --> D[logErrorConsole 'Path does not exist' + exit 1]
    C -- succeeds --> E[findTargetDirectories searchPath]
    B -- No --> E
    E --> F[configExists searchPath?]
    F -- Yes --> G[targets = searchPath]
    F -- No --> H[findDescendantConfigs searchPath]
    H --> G
    G --> I{targetDirs empty?}
    I -- Yes --> J[readHierarchicalConfig cwd]
    J --> K{hasConfigs?}
    K -- No --> L[logErrorConsole 'Not in Packmind project' + exit 1]
    K -- Yes --> M[log 'No targets found' + exit 0]
    I -- No --> N[collectGitInfo]
    N --> O{gitInfo null?}
    O -- Yes --> P[exit 1]
    O -- No --> Q[For each targetDir: readFullConfig]
    Q --> R{packages empty?}
    R -- Yes --> S[skip target]
    R -- No --> T[diffArtefacts with baseDirectory=targetDir]
    T --> U[targetResults.push targetRelativePath + diffs]
    U --> V[Build displayPathMap\n prefix = nodePath.relative cwd targetDir\n key = prefix + diff.filePath]
    V --> W[Merge allDiffs]
    W --> X[checkDiffs + displayDiffs]
    X --> Y{submit flag?}
    Y -- Yes --> Z[handleSubmission]
    Y -- No --> AA[exit 0]
Loading

Last reviewed commit: 2942f9a

Comment thread apps/cli/src/infra/commands/diffArtefactsHandler.spec.ts
Comment thread apps/cli/src/infra/commands/diffArtefactsHandler.ts
Comment thread apps/cli/src/application/utils/findNearestConfigDir.ts
cteyton and others added 5 commits March 12, 2026 15:10
…or git-relative path computation

String slicing on gitRoot was incorrect when targetDir shared a prefix
with gitRoot but wasn't a child (e.g. /foo/bar vs /foo/barista).
Use path.relative() consistently in diffArtefactsHandler, diffRemoveHandler,
and diffAddHandler, falling back to '/' when the target is outside the git root.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
… diffArtefactsHandler spec

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…figured

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…diff tests

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Comment thread apps/cli/src/infra/commands/diffArtefactsHandler.spec.ts
Comment thread apps/cli/src/infra/commands/diffArtefactsHandler.spec.ts Outdated
- Remove redundant mockResolvedValueOnce setup in 'passes correct
  baseDirectory for backend target' (beforeEach already provides it)
- Hoist diff1 to multi-target support scope and extract single-target
  tests into their own describe block with a dedicated beforeEach

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@cteyton
Copy link
Copy Markdown
Contributor Author

cteyton commented Mar 12, 2026

@greptile update your review

Comment thread apps/cli/src/infra/commands/diffArtefactsHandler.ts
vincent-psarga and others added 2 commits March 13, 2026 10:12
… command

- Validate that the path specified via --path exists before processing; exit with error if not found
- Replace console.error usages with logErrorConsole for consistent error formatting
- Remove error field from DiffHandlerDependencies
- Update tests accordingly

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Comment thread apps/cli/src/infra/commands/diffArtefactsHandler.spec.ts
vincent-psarga and others added 2 commits March 13, 2026 12:40
When running `packmind-cli diff rm --path <basePath> <filePath>`,
combine --path and the positional argument to build the full file path,
matching the behavior of `packmind-cli diff rm <basePath>/<filePath>`.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
When running `packmind-cli diff add --path <basePath> <filePath>`,
combine --path and the positional argument to build the full file path,
matching the behavior of `packmind-cli diff add <basePath>/<filePath>`.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Comment thread apps/cli/src/infra/commands/diffArtefactsHandler.ts
@sonarqubecloud
Copy link
Copy Markdown

@vincent-psarga vincent-psarga merged commit 2065ff1 into main Mar 13, 2026
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants