Skip to content

feat(cli): gitnexus remove <target> to unindex a registered repo by name or path (#664)#1003

Merged
magyargergo merged 5 commits into
abhigyanpatwari:mainfrom
azizur100389:feat/cli-remove-command
Apr 21, 2026
Merged

feat(cli): gitnexus remove <target> to unindex a registered repo by name or path (#664)#1003
magyargergo merged 5 commits into
abhigyanpatwari:mainfrom
azizur100389:feat/cli-remove-command

Conversation

@azizur100389

Copy link
Copy Markdown
Contributor

Problem (#664)

The existing clean command is cwd-scoped — it deletes the .gitnexus/ index for whichever repo you're standing in. That leaves two gaps:

  1. Remote cleanup — you can't remove a registered repo without cd-ing into it first (awkward in scripts; impossible if the working tree has been deleted).
  2. Alias/path addressingclean has no way to say "remove the one I registered as app-a" or "remove the one at /old/path/project"; it's strictly findRepo(cwd).

Issue #664 asks for a remove API that closes both gaps: identify a repo via a unique identifier (alias, name, or path), remove associated data atomically and idempotently.

What this PR adds

New command: gitnexus remove <target> [-f|--force]

<target> is resolved against ~/.gitnexus/registry.json via resolveRegistryEntry() with a tiered precedence:

  1. Absolute-path match (Windows: case-insensitive, matching feat(cli): analyze --name <alias> + duplicate-name guard for the repo registry #955's case-rules) — always unambiguous by registry construction
  2. Exact name match (case-insensitive) — if two entries share the alias (only possible after --allow-duplicate-name from analyze has no way to disambiguate repos with the same basename — -r <name> becomes ambiguous #829), throws RegistryAmbiguousTargetError with both paths surfaced
  3. MissRegistryNotFoundError with a disambiguated available-names list

No fuzzy / partial matching — destructive commands should be unambiguous and scriptable.

Behaviour

  • Default (no --force) is a dry-run that lists what would be deleted. Mirrors clean's confirmation-gate pattern.
  • --force deletes .gitnexus/ storage and unregisters from the global registry.
  • Idempotent on unknown targets: exits 0 with a warning, so remove X && analyze Y keeps working in scripts. This matches the Add a feature to remove a indexed repo #664 spec explicitly: "behave atomically and idempotently so retries are safe".
  • Atomicity order mirrors clean.ts: fs.rm first → unregisterRepo second. Partial failure leaves the registry pointing at a missing dir (recoverable by listRegisteredRepos({ validate: true }) on next read) rather than orphaning .gitnexus/ on disk.
  • --force semantics match clean -f (skip the safety prompt). Distinct from analyze --force (pipeline re-index) — no conflation because there's no pipeline in remove.

What's NOT in this PR (deliberate scope trim)

  • MCP tool wiringresolveRegistryEntry is exported specifically so the MCP-side remove tool is a trivial follow-up PR that reuses the same resolver. Keeping this PR CLI-only keeps the diff reviewable.
  • Path-based cleanclean remains cwd-scoped. The two commands now cover complementary workflows (clean for "I'm here, delete this"; remove for "delete that one over there").

Tests

Unit (7 new in test/unit/repo-manager.test.ts)

  • Path tier beats name tier (never ambiguous by construction)
  • Resolve by unique name
  • Name match is case-insensitive
  • Path match is case-insensitive on Windows only (POSIX stays strict)
  • Ambiguous-name target throws RegistryAmbiguousTargetError with both paths in the message
  • Not-found target throws RegistryNotFoundError with a disambiguated available-names hint (app (path) when app appears twice)
  • Empty registry → helpful "no repositories registered" hint

Integration (2 new in test/integration/cli-e2e.test.ts)

  1. Happy path (~60s): analyze --name alias-a → dry-run remove alias-a (verify state unchanged) → remove alias-a --force (verify .gitnexus/ gone + registry empty) → remove alias-a again (verify exit 0 + "Nothing to remove" warning — idempotent)
  2. Ambiguity path (~120s): register two repos under --name shared with --allow-duplicate-nameremove shared --force errors with exit 1 and both paths surfaced, registry unchangedremove <absolute-path> --force succeeds, leaves the other survivor

Both tests sandbox via GITNEXUS_HOME so they never touch the developer's real ~/.gitnexus/registry.json. Structural assertions (entry count, basename, path distinctness) avoid the path-realpath flakiness that bit #955 on cross-platform CI.

Verification

From gitnexus/:

npx tsc --noEmit                                              # clean
npx vitest run test/unit/repo-manager.test.ts                 # 37/37 pass
npx vitest run test/integration/cli-e2e.test.ts -t remove     # 2/2 pass (24s)
npm run test:unit                                             # 4183 pass, 2 pre-existing env failures unchanged

Pre-commit hook (lint-staged + eslint --fix + prettier --write + gitnexus tsc) passes.

Backwards compatibility

  • No schema changes. Registry format unchanged. Old registry.json files load unchanged.
  • No existing command modified. clean, analyze, list, status are all untouched. Only a new command is added.
  • No new required flags on any existing surface. Pure addition.
  • Revert safety: pure code revert; nothing on disk to unwind.

Closes #664.

…ame or path (abhigyanpatwari#664)

Add a `remove` CLI command that deletes the `.gitnexus/` index AND
unregisters a repo from the global registry (~/.gitnexus/registry.json),
addressing the lifecycle gap flagged in abhigyanpatwari#664: previously users had to
cd into the repo to run `clean`, and there was no path-based or
alias-based remove for an already-deleted working tree.

- New command `gitnexus remove <target> [-f|--force]`. `<target>` is
  alias / basename-derived name / remote-inferred name / absolute path.
- New helper `resolveRegistryEntry(entries, target)` in repo-manager.ts
  with path > name precedence; throws RegistryNotFoundError or
  RegistryAmbiguousTargetError (typed, `kind`-discriminated).
- Atomicity mirrors `clean`: fs.rm first, then unregisterRepo; partial
  failures self-heal on next `listRegisteredRepos({ validate: true })`.
- Idempotent on unknown targets (exit 0 with warning) per the abhigyanpatwari#664
  spec: "behave atomically and idempotently so retries are safe".
- `--force` uses `clean`-style confirmation-skip semantics — distinct
  from `analyze --force` (pipeline re-index); here there is no pipeline
  so no conflation.
- 7 new unit tests cover resolver precedence, case sensitivity,
  ambiguity, and not-found hints; 2 integration tests cover the real
  CLI -> registry -> filesystem chain including the --allow-duplicate-name
  (abhigyanpatwari#829) ambiguity case.
@vercel

vercel Bot commented Apr 20, 2026

Copy link
Copy Markdown

@azizur100389 is attempting to deploy a commit to the NexusCore Team on Vercel.

A member of the Team first needs to authorize it.

@github-actions

github-actions Bot commented Apr 20, 2026

Copy link
Copy Markdown
Contributor

CI Report

All checks passed

Pipeline Status

Stage Status Details
✅ Typecheck success tsc --noEmit
✅ Tests success unit tests, 3 platforms
✅ E2E success gitnexus-web changes only

Test Results

Tests Passed Failed Skipped Duration
7024 6927 0 97 285s

✅ All 6927 tests passed

97 test(s) skipped — expand for details
  • Swift MethodExtractor > isTypeDeclaration > recognizes class_declaration
  • Swift MethodExtractor > isTypeDeclaration > recognizes protocol_declaration
  • Swift MethodExtractor > isTypeDeclaration > rejects import_declaration
  • Swift MethodExtractor > visibility > extracts public method
  • Swift MethodExtractor > visibility > extracts private method
  • Swift MethodExtractor > visibility > defaults to internal when no modifier
  • Swift MethodExtractor > protocol methods > marks protocol method as abstract
  • Swift MethodExtractor > static and class methods > detects static func as isStatic
  • Swift MethodExtractor > static and class methods > detects class func as isStatic
  • Swift MethodExtractor > parameters > extracts parameters with types and default values
  • Swift MethodExtractor > return type > extracts return type from -> annotation
  • Swift MethodExtractor > annotations > extracts @objc attribute
  • Swift MethodExtractor > isFinal > detects final func
  • Swift MethodExtractor > isFinal > is false when not final
  • Swift MethodExtractor > isAsync > detects async func
  • Swift MethodExtractor > isOverride > detects override method
  • buildTypeEnv > constructor inference (Tier 1 fallback) > lookupClassByName regression coverage > Swift lookupClassByName regression coverage > Swift cross-file constructor inference uses lookupClassByName
  • buildTypeEnv > constructor inference (Tier 1 fallback) > lookupClassByName regression coverage > Swift lookupClassByName regression coverage > Swift explicit init inference uses lookupClassByName
  • buildTypeEnv > constructor inference (Tier 1 fallback) > lookupClassByName regression coverage > Swift lookupClassByName regression coverage > Swift cross-file constructor inference does not bind plain functions
  • buildTypeEnv > known limitations (documented skip tests) > Ruby block parameter: users.each { |user| } — closure param inference, different feature
  • Swift constructor-inferred type resolution > detects User and Repo classes, both with save methods
  • Swift constructor-inferred type resolution > resolves user.save() to Models/User.swift via constructor-inferred type
  • Swift constructor-inferred type resolution > resolves repo.save() to Models/Repo.swift via constructor-inferred type
  • Swift constructor-inferred type resolution > emits exactly 2 save() CALLS edges (one per receiver type)
  • Swift self resolution > detects User and Repo classes, each with a save function
  • Swift self resolution > resolves self.save() inside User.process to User.save, not Repo.save
  • Swift parent resolution > detects BaseModel and User classes plus Serializable protocol
  • Swift parent resolution > emits EXTENDS edge: User → BaseModel
  • Swift parent resolution > emits IMPLEMENTS edge: User → Serializable (protocol conformance)
  • Swift cross-file User.init() inference > resolves user.save() via User.init(name:) inference
  • Swift cross-file User.init() inference > resolves user.greet() via User.init(name:) inference
  • Swift return type inference > detects User class and getUser function
  • Swift return type inference > detects save function on User (Swift class methods are Function nodes)
  • Swift return type inference > resolves user.save() to User#save via return type of getUser() -> User
  • Swift return-type inference via function return type > resolves user.save() to User#save via return type of getUser()
  • Swift return-type inference via function return type > user.save() does NOT resolve to Repo#save
  • Swift return-type inference via function return type > resolves repo.save() to Repo#save via return type of getRepo()
  • Swift implicit imports (cross-file visibility) > detects UserService class in Models.swift
  • Swift implicit imports (cross-file visibility) > resolves UserService() constructor call across files (no explicit import)
  • Swift implicit imports (cross-file visibility) > resolves service.fetchUser() member call across files
  • Swift implicit imports (cross-file visibility) > creates IMPORTS edges between files in the same module
  • Swift extension deduplication > detects Product class
  • Swift extension deduplication > resolves Product() constructor despite extension creating duplicate class node
  • Swift extension deduplication > resolves product.save() to Product.swift (primary definition)
  • Swift constructor call fallback (no new keyword) > resolves OCRService() as constructor call across files
  • Swift constructor call fallback (no new keyword) > resolves ocr.recognize() member call via constructor-inferred type
  • Swift export visibility (internal vs private) > resolves PublicService() constructor across files
  • Swift export visibility (internal vs private) > resolves internalHelper() across files (internal = module-scoped)
  • Swift if let / guard let binding resolution > detects User and Repo classes
  • Swift if let / guard let binding resolution > resolves user.save() inside if-let to User#save
  • Swift if let / guard let binding resolution > resolves repo.save() inside guard-let to Repo#save
  • Swift if let / guard let binding resolution > user.save() in if-let does NOT resolve to Repo#save
  • Swift await / try expression unwrapping > resolves user.save() via await fetchUser() return type
  • Swift await / try expression unwrapping > resolves repo.save() via try parseRepo() return type
  • Swift await / try expression unwrapping > detects fetchUser and parseRepo as functions
  • Swift for-in loop element type inference > detects User and Repo classes
  • Swift for-in loop element type inference > creates implicit import edges between files
  • Swift field-type resolution > detects classes and their properties
  • Swift field-type resolution > emits HAS_PROPERTY edges from class to field
  • Swift field-type resolution > resolves field-chain call user.address.save() → Address#save
  • Swift field-type resolution > emits ACCESSES edges for field reads in chains
  • Swift field-type resolution > populates field metadata (visibility, declaredType) on Property nodes
  • Swift call-result binding > resolves call-result-bound method call user.save() → User#save
  • Swift call-result binding > getUser() is present as a defined function
  • Swift call-result binding > emits processUser -> getUser CALLS edge for let-assigned free function call
  • Swift method enrichment > detects Animal protocol and Dog class
  • Swift method enrichment > emits IMPLEMENTS edge Dog -> Animal
  • Swift method enrichment > emits HAS_METHOD edges for Dog methods
  • Swift method enrichment > marks protocol Animal.speak as isAbstract
  • Swift method enrichment > marks Dog.speak as NOT isAbstract
  • Swift method enrichment > marks breathe as isFinal
  • Swift method enrichment > marks classify as isStatic
  • Swift method enrichment > captures @objc annotation on breathe
  • Swift method enrichment > populates parameterTypes for classify(_ name: String)
  • Swift method enrichment > records parameterCount for classify
  • Swift method enrichment > records returnType for speak
  • Swift method enrichment > resolves dog.speak() CALLS edge
  • Swift method enrichment > resolves Dog.classify("dog") CALLS edge
  • Swift abstract dispatch > detects Repository protocol and SqlRepository class
  • Swift abstract dispatch > emits IMPLEMENTS edge SqlRepository -> Repository
  • Swift abstract dispatch > emits HAS_METHOD edges for Repository.find and Repository.save
  • Swift abstract dispatch > emits HAS_METHOD edges for SqlRepository.find and SqlRepository.save
  • Swift abstract dispatch > marks base Repository.find as isAbstract
  • Swift abstract dispatch > marks base Repository.save as isAbstract
  • Swift abstract dispatch > marks concrete SqlRepository.find as NOT isAbstract
  • Swift abstract dispatch > resolves repo.find(id: 42) CALLS edge
  • Swift abstract dispatch > resolves repo.save(entity: user) CALLS edge
  • Swift abstract dispatch > populates parameterTypes for Repository.find
  • Swift abstract dispatch > populates parameterTypes for Repository.save
  • Swift abstract dispatch > records returnType for SqlRepository.find
  • Swift abstract dispatch > emits METHOD_IMPLEMENTS edges from SqlRepository methods → Repository protocol methods
  • Swift overloaded method disambiguation > detects 2 distinct find Method nodes on SqlRepository
  • Swift overloaded method disambiguation > emits METHOD_IMPLEMENTS edges for both find overloads
  • Swift overloaded method disambiguation > emits METHOD_IMPLEMENTS edge for save
  • Swift overloaded method disambiguation > emits exactly 3 METHOD_IMPLEMENTS edges total
  • Swift Child extends Parent — inherited method resolution (SM-9) > detects Parent and Child classes
  • Swift Child extends Parent — inherited method resolution (SM-9) > resolves c.parentMethod() to Parent.parentMethod via first-wins MRO walk

Code Coverage

Tests

Metric Coverage Covered Base Delta Status
Statements 73.58% 18930/25727 73.63% 📉 -0.0 🔴 ██████████████░░░░░░
Branches 62.31% 12029/19302 62.33% 📉 -0.0 🔴 ████████████░░░░░░░░
Functions 78.92% 1835/2325 78.9% 📈 +0.0 🟢 ███████████████░░░░░
Lines 76.2% 17170/22532 76.28% 📉 -0.1 🔴 ███████████████░░░░░

📋 View full run · Generated by CI

@evander-wang

evander-wang commented Apr 21, 2026

Copy link
Copy Markdown
Contributor

@azizur100389 Since remove now supports custom path input, cross-platform path normalization needs to be addressed, but also for existing analyze and remove. Additionally, consider backward compatibility: paths stored by older versions of removeRepo (before normalization) should still be matchable when remove looks them up.

@magyargergo

Copy link
Copy Markdown
Collaborator

I agree with @evander-wang please look into this plus the following findings.

@github-actions

github-actions Bot commented Apr 21, 2026

Copy link
Copy Markdown
Contributor

Claude finished @magyargergo's task in 4m 34s —— View job


PR Review: feat(cli): gitnexus remove <target> (#1003)


Production-ready bar for this PR

Based on AGENTS.md, GUARDRAILS.md, CONTRIBUTING.md, TESTING.md, and the affected area (gitnexus/src/cli/, gitnexus/src/storage/repo-manager.ts):

  1. No mutations to existing commandsclean, analyze, list, status must be unchanged; only additions are permitted.
  2. Registry contract preservedregistry.json schema and field semantics (name, path, storagePath, indexedAt, lastCommit, stats) must remain backward-compatible. No silent schema changes.
  3. Resolver contracts correctresolveRegistryEntry must implement exact-match semantics (path tier > name tier, no fuzzy/partial), throw typed errors on ambiguity and miss, and be free of side effects.
  4. Idempotent/atomic deletionfs.rm before unregisterRepo; idempotent on unknown targets (exit 0); ambiguous targets exit 1 with both paths surfaced; no state mutation before resolution succeeds.
  5. Cross-platform path handling — Windows case-insensitive path comparison must be consistent across all registry operations that touch path equality.
  6. CI must passtsc --noEmit clean, npm test passes on all 3 platforms, no unexplained stage failures.
  7. No secrets, machine-specific paths, or blind renames — per GUARDRAILS.md non-negotiables.
  8. Tests under gitnexus/test/ — unit tests for the new pure logic (resolveRegistryEntry); integration tests for the CLI command end-to-end.
  9. PR title follows conventional commit format — enforced by pr-labeler.yml.
  10. Rollback safe — pure code revert, no schema migration required.

Findings

1. Correctness & functional completeness

[NON-BLOCKING] CI Tests stage is ❌ with 0 test failures

  • Evidence: CI report comment (2026-04-20T23:42:53Z): Stage Tests = failure / unit tests, 3 platforms, but the test results table shows 7006 total, 6909 passed, 0 failed, 97 skipped. Coverage shows Statements: 73.58% (base 73.61%, Δ −0.03%) 🔴 and Lines: 76.21% (base 76.26%, Δ −0.05%) 🔴.
  • Why it matters: A failing CI stage with zero test failures typically signals a coverage-threshold exit or a platform-specific runner failure (possibly Windows). The new remove.ts (~50 executable lines) is tested only by integration tests (child-process spawns), not unit tests — integration test coverage may not be attributed to the parent coverage run. This is the most likely root cause of the 🔴 deltas.
  • Recommendation: Confirm whether vitest.config.ts configures a minimum coverage threshold and whether the failing job is on a specific platform. If it's purely a coverage-threshold gate, add lightweight unit tests for removeCommand's RegistryNotFoundError / RegistryAmbiguousTargetError / dry-run branches using vi.mock on repo-manager and fs/promises.
  • Confidence: High (contradiction between stage result and test results is verified from the CI report).

[NON-BLOCKING] unregisterRepo omits Windows case-insensitive path comparison

  • Evidence: repo-manager.ts:429: entries.filter((e) => path.resolve(e.path) !== resolved) — strict ===. Compare with registerRepo:358 and resolveRegistryEntry:513, both of which use process.platform === 'win32' ? a.toLowerCase() === b.toLowerCase() : a === b.
  • Why it matters: In the current remove.ts flow (unregisterRepo(entry.path) at remove.ts:82), entry.path is already the path.resolve()d string stored by registerRepo, so the double-resolve produces the same string and the strict === happens to work. However, resolveRegistryEntry is exported specifically for future MCP wiring; if an MCP caller uses resolveRegistryEntry + unregisterRepo directly and supplies a differently-cased path on Windows, unregisterRepo will silently no-op rather than remove the entry. This is a pre-existing inconsistency that this PR's export of resolveRegistryEntry makes more likely to surface.
  • Recommendation: Add the same process.platform === 'win32' lowercase guard to unregisterRepo:429, consistent with the other two callers. Fix this →
  • Confidence: High (verified from source).

[NON-BLOCKING] Double readRegistry() in the --force deletion path introduces a stale-read window

  • Evidence: remove.ts:42 reads the registry for dry-run display. remove.ts:82 calls unregisterRepo(entry.path), which internally calls readRegistry() again at repo-manager.ts:428. The PR acknowledges this ("The comment in remove.ts explains this is intentional").
  • Why it matters: Between the two reads, a concurrent analyze could register a new entry. The second write would silently lose that entry. The PR correctly notes this is self-healing on next listRegisteredRepos({ validate: true }), but the lost-registration scenario isn't mentioned in user-facing docs.
  • Recommendation: Acceptable given the single-writer LadybugDB model and the documented self-healing path. No change required — just document the known limitation in a code comment at remove.ts:82 or in a note near unregisterRepo.
  • Confidence: High (code trace verified).

2. Code clarity & clean code

No issues found in this area based on the diff and reviewed context. Naming is consistent with existing conventions (resolveRegistryEntry, RegistryNotFoundError, RegistryAmbiguousTargetError). No dead code, no // removed comments, no barrel re-export regressions. resolveRegistryEntry is a pure function with no side effects, correctly placed in repo-manager.ts alongside the other registry operations. The lazy-action wiring in index.ts:87–93 matches the existing pattern exactly.

3. Test coverage & change safety

[NIT] removeCommand itself has no unit test; integration test assertions on dry-run output are minimal

  • Evidence: gitnexus/test/unit/repo-manager.test.ts covers resolveRegistryEntry (7 tests, all relevant error branches). But remove.ts's removeCommand (reading registry, dispatching errors, dry-run print, fs.rm + unregister) has no unit test — it's only exercised via full integration test spawns at cli-e2e.test.ts:363,438. The dry-run assertion (cli-e2e.test.ts:401) checks only /Run with --force/i — it doesn't assert that entry.name, entry.path, or entry.storagePath appear in the output.
  • Why it matters: The dry-run output format (remove.ts:68–71) can silently regress without the integration test catching it; the assertion only confirms the command returned exit 0 and mentioned --force.
  • Recommendation: NIT only — the resolveRegistryEntry unit tests cover the new pure logic comprehensively, and the integration tests cover the deletion and idempotency paths. Strengthening the dry-run assertion would be a small improvement.
  • Confidence: High (verified from test source).

4. Performance

No issues found in this area based on the diff and reviewed context. readRegistry() reads a small JSON file. resolveRegistryEntry is O(n) with n = registry entries; no O(n²) patterns. fs.rm({ recursive: true, force: true }) is the standard filesystem cleanup pattern.

5. Operational risk

[NON-BLOCKING] Path-matching asymmetry between resolveRegistryEntry (case-insensitive on Windows) and unregisterRepo (strict equality) exposes MCP callers

(See finding in §1 above — same evidence applies here from an operational-risk angle.)

Rollback safety: ✅ Pure code revert. No schema changes, no migration needed.

MCP contract compatibility: ✅ No existing MCP tools are modified. resolveRegistryEntry is exported but not yet wired to any MCP tool — the contract is new, not a change to an existing one.

LadybugDB single-writer invariant:fs.rm operates on .gitnexus/ only, not on the LadybugDB file directly. No LadybugDB writes.

Embeddings preservation:fs.rm(entry.storagePath, { recursive: true, force: true }) at remove.ts:81 intentionally deletes the entire .gitnexus/ tree including embeddings — this is correct for a "remove" command. Embeddings concern is not applicable.

Secrets/machine-specific paths: ✅ None committed. GITNEXUS_HOME env var used correctly in tests for sandboxing.

6. Maintainability

No issues found in this area based on the diff and reviewed context. The change respects Pipeline Phase DAG boundaries (no ingestion code touched). resolveRegistryEntry is correctly placed in repo-manager.ts, not duplicated between CLI and server. The next contributor adding MCP remove tool support has a clear path: import resolveRegistryEntry from repo-manager.ts, reuse the same resolver, wire up unregisterRepo. ARCHITECTURE.md §Storage documents repo-manager.ts as the owner of registry operations.


Non-negotiables check

Check Status
No secrets/machine-specific paths ✅ Pass
No blind find-and-replace renames ✅ Pass (no renames)
No HIGH/CRITICAL impact ignored ✅ Pass (pure addition)
PR title follows conventional commit format feat(cli): gitnexus remove <target>…
PolyForm license untouched ✅ Pass
tsc --noEmit ✅ CI typecheck passed
vitest run ⚠️ CI Tests stage ❌ despite 0 failures — needs explanation

What I could not fully verify

  • Root cause of CI Tests stage failure — cannot determine from the diff whether the failure is a coverage-threshold exit, a platform-specific (Windows) runner error, or an infrastructure flake. The vitest.config.ts coverage thresholds would need to be read to confirm.
  • Backward compatibility of storagePath values stored by older gitnexus versions — cannot verify without registry.json samples from prior versions; however, registerRepo has always called getStoragePaths(resolved) (which uses path.join), making this low-risk.
  • macOS symlink realpath behavior in integration testsos.tmpdir() on macOS returns /var/folders/... (symlink to /private/var/folders/...). The integration tests use structural assertions (entry count, basename) rather than exact paths, which sidesteps this correctly.

Final verdict

⚠️ Merge after addressing blocking items.

The logic is correct, the resolver contract is solid, tests are meaningful, and the implementation is clean. One item must be resolved before merge:

  1. CI Tests stage is ❌ — even with 0 test failures, a red CI stage must be explained and resolved before merging to main. Confirm whether this is a coverage-threshold exit and, if so, either add unit tests for removeCommand or adjust the threshold with a documented justification.

The Windows unregisterRepo case-sensitivity inconsistency is pre-existing and low-risk in the current flow, but worth patching before the MCP follow-up PR ships.

@magyargergo

Copy link
Copy Markdown
Collaborator

@azizur100389 Please make sure CI is healthy 🙏

…tforms (abhigyanpatwari#1003 review)

Address review feedback from @evander-wang and @magyargergo on PR abhigyanpatwari#1003
plus the Windows + macOS CI failure (same root cause).

Problem:
- macOS: /var is a symlink to /private/var. `path.resolve` does NOT
  follow symlinks, so a child running analyze in /var/folders/X stores
  /private/var/folders/X (realpath from OS cwd) but an outer caller
  passing the symlink form misses.
- Windows: GitHub runners surface tmpdirs in 8.3 short-name form
  (RUNNERA~1) while process.cwd() returns the long form (runneradmin).
  Same divergence.

Fix: new `canonicalizePath(p)` helper wraps `path.resolve` plus
`fs.realpathSync.native`, falling back to `path.resolve` when the path
doesn't exist (preserves idempotent-on-missing semantics needed by
`remove <unknown>`). Applied at 3 call-sites — registerRepo,
unregisterRepo, resolveRegistryEntry — canonicalising BOTH the input
and each stored `entry.path` at compare time. That last bit is the
backward-compat story: registries written by older versions
(pre-canonicalisation) still match correctly, so we don't need a
migration script.

Test side: the ambiguous-target integration test now reads the path
from the registry snapshot rather than passing the outer `repoA`
variable directly, so it exercises the registry contract regardless of
which path form the platform stores. 4 new unit tests cover the helper
(idempotent, fallback-on-missing, absolute-for-relative) plus the
backward-compat resolver path.
@azizur100389

Copy link
Copy Markdown
Contributor Author

@evander-wang @magyargergo — thanks for the review. Pushed c5eceba0 which addresses the path-normalisation feedback and the Windows + macOS CI failure (same root cause).

Root cause of the CI red

The failing integration test was hitting cross-platform path divergence:

  • macOS: /var is a symlink to /private/var. When analyze runs with cwd /var/folders/.../repo, the child stores /private/var/folders/.../repo (realpath from OS cwd semantics), but the test was passing the symlink form back to remove, which missed.
  • Windows: GitHub runners surface tmpdirs in 8.3 short-name form (RUNNERA~1), while process.cwd() returns the long form (runneradmin). Same divergence.

path.resolve alone doesn't follow symlinks or expand 8.3 short names, so both forms were coexisting as distinct keys in the registry comparison.

Fix

New helper canonicalizePath(p) in repo-manager.ts:

export const canonicalizePath = (p: string): string => {
  const resolved = path.resolve(p);
  try {
    return realpathSync.native(resolved);
    // macOS: /var → /private/var; Windows: RUNNERA~1 → runneradmin
  } catch {
    return resolved;
    // path doesn't exist on disk — fall back, preserves
    // resolveRegistryEntry's idempotent-on-missing semantics
  }
};

Applied at 3 call-sites:

  • registerRepo — canonicalises input before storing; canonicalises each existing entry at match time too
  • unregisterRepo — canonicalises both sides symmetric with register
  • resolveRegistryEntry path tier — canonicalises input AND each stored entry.path at compare time

Backward compatibility (addressing @evander-wang's second point)

Because resolveRegistryEntry canonicalises the stored entry.path (not just the new input) at compare time, registries written by older versions — where registerRepo only ran path.resolve — still match correctly. Both pre- and post-fix entries converge to the same key at compare time, so no migration script is needed.

Test side

  • Ambiguous-target integration test now reads the path from the registry snapshot rather than passing the outer repoA variable directly. Belt-and-suspenders against platform quirks, complementing the code fix.
  • 4 new unit tests cover canonicalizePath (idempotent, fallback-on-missing, absolute-for-relative) and the backward-compat resolver path (matches a stored non-canonical entry when target is passed in canonical form).

Local verification

CI should light up green shortly.

Comment on lines +80 to +89
try {
await fs.rm(entry.storagePath, { recursive: true, force: true });
await unregisterRepo(entry.path);
console.log(`Removed: ${entry.name}`);
console.log(` Path: ${entry.path}`);
console.log(` Storage: ${entry.storagePath}`);
} catch (err) {
console.error(`Failed to remove ${entry.name}:`, err);
process.exit(1);
}

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please be very careful here! We don't want to remove the physical path of the code base. We can safely remove files/folders int the .gitnexus folder but outside is prohibited. Please introduce safe guards here.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch, thank you. Addressed in 610ee9b9 with a guard that blocks destructive fs.rm whenever the registry entry's storagePath isn't the canonical <entry.path>/.gitnexus subfolder.

The threat model

~/.gitnexus/registry.json is a plain-text user-writable file. A corrupted or hand-edited entry could plausibly end up with:

  • storagePath === entry.path (the repo root → catastrophic: fs.rm recursively wipes the working tree)
  • storagePath === "" (path.resolve resolves to cwd → rm cwd)
  • storagePath pointing at a parent dir, sibling dir, or anywhere else

fs.rm(recursive: true, force: true) on any of those is a runtime disaster.

Fix shape

New UnsafeStoragePathError + exported assertSafeStoragePath() in repo-manager.ts. Pure lexical string check (Windows case-insensitive) asserting entry.storagePath === path.join(entry.path, '.gitnexus'). Does NOT depend on the paths existing — it's a structural integrity check on the registry, not a filesystem probe.

Audit & sibling fix

Before committing I audited every fs.rm(...storagePath...) site in the codebase:

Site Source of storagePath Safety
remove.ts entry.storagePath from registry guarded
clean.ts --all entry.storagePath from registry (same pattern) guarded (found during the audit — same vulnerability, fixed in the same commit)
clean.ts default findRepo(cwd) → lexically recomputed ✅ safe by construction
server/api.ts getStoragePath(entry.path) → lexically recomputed ✅ safe by construction

clean --all skips poisoned entries with a warning and continues with the rest of the batch — preserves its existing per-repo error-tolerance semantics (one bad entry doesn't halt cleanup).

Tests

  • 8 unit tests on the guard: valid <repo>/.gitnexus, repo-root-as-storage (catastrophic case), parent-as-storage, empty storage (→ cwd), totally-unrelated path, sibling .gitnexus (right basename, wrong parent), error payload shape, Windows case-insensitive acceptance.
  • 2 integration tests: poisons a registry entry's storagePath to the repo root, runs the destructive command, asserts (a) exit code, (b) the working tree + .git/ + .gitnexus/ all survive on disk, (c) registry state is correct (poisoned entry retained, valid siblings cleaned). One test covers remove, one covers clean --all with a mixed good/bad registry.

Local verification

  • tsc --noEmit clean
  • repo-manager.test.ts 49/49
  • cli-e2e.test.ts -t "remove|clean --all" 4/4 (3 remove + 1 new clean --all poisoned)
  • Full unit suite 4177 pass + 2 pre-existing git-utils env failures (unchanged, unrelated)

CI on 610ee9b9 should light up green shortly.

…zePath (abhigyanpatwari#1003 CI)

Follow-up to c5eceba. The previous commit canonicalised the repo path
at BOTH write-time AND compare-time in registerRepo — that expanded
Windows 8.3 short names (RUNNER~1) to long names (runneradmin) when
storing `entry.path`. Pre-existing abhigyanpatwari#829 unit tests that assert
`path.resolve(err.existingPath) === path.resolve(tmpPath)` then broke
because `tmpPath` is still short-form (path.resolve doesn't expand
8.3) while `entry.path` was long-form (canonicalizePath does).

Fix: split storage from comparison.
- entry.path stores `path.resolve(repoPath)` — whatever form the
  caller passed. `list` output and error messages show the path the
  user typed.
- All compare points (existing-entry lookup in registerRepo, the
  collision guard, unregisterRepo, resolveRegistryEntry path tier)
  canonicalise BOTH sides via `canonicalizePath`. That is where the
  /var ↔ /private/var and RUNNER~1 ↔ runneradmin divergence actually
  matters.

Net effect: storage is tolerant (preserves user input), matching is
strict (canonical-vs-canonical). Pre-existing abhigyanpatwari#829 tests stay green
because `err.existingPath` is unchanged from what `path.resolve` gives
back; the cross-platform CI failure from abhigyanpatwari#1003 stays fixed because
every comparison path goes through `canonicalizePath`.
@azizur100389

Copy link
Copy Markdown
Contributor Author

Follow-up fix in 608299c2 — the prior commit canonicalised paths at both write-time AND compare-time in registerRepo. The write-time canonicalisation broke pre-existing #829 unit tests on CI because it expanded Windows 8.3 short names (RUNNER~1) to long names (runneradmin) in entry.path, which made the test's path.resolve(err.existingPath) === path.resolve(tmpPath) assertion fail (tmpPath stays 8.3 because path.resolve doesn't expand short names; entry.path was now long-form).

Revised shape: split storage from comparison cleanly.

  • Storage: entry.path = path.resolve(repoPath) — preserves whatever form the caller passed. list output and error messages show the path the user actually typed (not a runtime-resolved form they may never have seen).
  • Comparison: every compare point (registerRepo existing-entry lookup, collision guard, unregisterRepo filter, resolveRegistryEntry path tier) canonicalises BOTH sides via canonicalizePath.

Net effect: storage is tolerant (preserves user input), matching is strict (canonical-vs-canonical). This keeps pre-existing #829 tests green AND still fixes the cross-platform matching divergence from #1003.

Local verification: tsc clean · repo-manager.test.ts 41/41 · cli-e2e.test.ts -t remove 2/2 · cli-e2e.test.ts -t "analyze --name" 1/1. Waiting on CI to confirm.

@magyargergo

Copy link
Copy Markdown
Collaborator

@azizur100389 please look into the inline comment I left before

…epo>/.gitnexus (abhigyanpatwari#1003 review)

Address @magyargergo's inline review finding on remove.ts:89 and the
sibling vulnerability in clean.ts --all (caught during a pre-commit
safety audit). ~/.gitnexus/registry.json is a user-writable plain-text
file, so a corrupted or hand-edited entry could point storagePath at
the repo root (catastrophic: rm the working tree), an empty string
(→ cwd), a parent dir, or anywhere else. fs.rm(recursive: true,
force: true) on any of those is a runtime disaster.

- New UnsafeStoragePathError + exported assertSafeStoragePath() in
  repo-manager.ts. Pure lexical string check (Windows-case-
  insensitive) asserting entry.storagePath === path.join(entry.path,
  '.gitnexus').
- Guard wired into BOTH destructive registry-trusting sites:
  - remove.ts: exit 1 with actionable hint
  - clean.ts --all: skip the poisoned entry with a warning and
    continue (preserves existing per-repo error tolerance — one bad
    entry doesn't halt the batch)
- clean.ts default path and server/api.ts are safe-by-construction
  (they recompute storagePath from findRepo / getStoragePath rather
  than trusting the registry field).
- 8 unit tests cover the guard (valid, repo-root, parent, empty,
  unrelated, sibling, error payload, Windows case).
- 2 integration tests prove the full CLI path: remove-poisoned exits
  1 without touching the working tree; clean --all with a poisoned
  sibling entry cleans the good entry, skips the bad one, and leaves
  the poisoned repo intact.
@github-actions

github-actions Bot commented Apr 21, 2026

Copy link
Copy Markdown
Contributor

Claude finished @magyargergo's task in 5m 7s —— View job


PR Review: feat(cli): gitnexus remove <target> (#1003)

Tasks

  • Post initial comment
  • Read repo docs (AGENTS.md, GUARDRAILS.md, CONTRIBUTING.md, TESTING.md, ARCHITECTURE.md)
  • Read changed files (remove.ts, clean.ts, index.ts, repo-manager.ts, tests)
  • Define production-ready bar
  • Review diff against bar
  • Post final review

Production-ready bar for this PR

Based on AGENTS.md, GUARDRAILS.md, CONTRIBUTING.md, TESTING.md, and the affected area (gitnexus/src/cli/, gitnexus/src/storage/repo-manager.ts):

  1. No mutations to existing commandsclean, analyze, list, status are untouched; only additions (plus the safety-guard backfill into clean --all) are permitted.
  2. Registry contract preservedregistry.json schema and field semantics (name, path, storagePath, indexedAt, lastCommit, stats) remain backward-compatible. No silent schema changes.
  3. Resolver contracts correctresolveRegistryEntry implements exact-match semantics (path tier > name tier, no fuzzy/partial), throws typed errors on ambiguity and miss, is side-effect-free, and is exported for the planned MCP follow-up.
  4. Safety guard for destructive operationsfs.rm must only operate on the canonical <repo>/.gitnexus subfolder. Guard must cover both remove and clean --all (the two paths that read storagePath from the registry rather than re-deriving it).
  5. Idempotent/atomic deletionfs.rm before unregisterRepo; idempotent on unknown targets (exit 0); ambiguous targets exit 1 with full disambiguation; no state mutation before resolution succeeds.
  6. Cross-platform path handling — Windows case-insensitive path comparison and macOS /var/private/var symlink expansion consistent across all three registry operations that touch path equality (registerRepo, unregisterRepo, resolveRegistryEntry).
  7. CI must passtsc --noEmit clean, npm test on all 3 platforms green.
  8. No secrets, machine-specific paths — per GUARDRAILS.md non-negotiables.
  9. Unit + integration tests under gitnexus/test/ — pure logic covered by unit tests; CLI command end-to-end covered by integration tests.
  10. PR title conventional commit format — enforced by pr-labeler.yml. ✅ feat(cli): …

Findings

1. Correctness & functional completeness

No issues found in this area based on the diff and reviewed context.

Verified:

  • removeCommand resolves entry via resolveRegistryEntry, dry-runs on no --force, calls assertSafeStoragePath before any disk I/O, then fs.rmunregisterRepo in that atomic order. ✅ (remove.ts:44–109)
  • RegistryNotFoundError → exits 0 with warning (idempotent). RegistryAmbiguousTargetError → exits 1 with disambiguation. Neither path mutates state before the error fires. ✅ (remove.ts:49–65)
  • resolveRegistryEntry is a pure function: path tier first (canonicalizePath on both sides), name tier second (case-insensitive), miss throws RegistryNotFoundError. No fuzzy/partial matching. ✅ (repo-manager.ts:645–686)
  • assertSafeStoragePath correctly uses path.resolve on both sides — consistent with how registerRepo writes the fields (entry.path = path.resolve(repoPath), entry.storagePath = path.join(path.resolve(repoPath), '.gitnexus')). The structural check is sound regardless of canonical form because both fields share the same path.resolve base. ✅ (repo-manager.ts:610–620)
  • clean --all guard correctly skips poisoned entries and continues (preserves existing per-repo error-tolerance semantics). ✅ (clean.ts:44–52)
  • clean default (cwd-scoped) path does NOT call assertSafeStoragePath — correct, because storagePath is re-derived via findRepo(cwd) → loadRepo → getStoragePaths, never read from the registry. Safe by construction. ✅ (clean.ts:67–88)
  • index.ts:87–93: lazy-action wiring matches the existing pattern exactly. ✅

2. Code clarity & clean code

No issues found in this area based on the diff and reviewed context.

Naming is consistent with existing conventions. The inline comments explaining canonicalizePath, atomicity order, and the backward-compat note are appropriately detailed for non-obvious behavior. No dead code, no // removed comments, no barrel re-export regressions. Imports in remove.ts are named, not from a barrel.

3. Test coverage & change safety

[NIT] Dry-run integration assertion covers only the --force hint, not the output structure

  • Evidence: cli-e2e.test.ts:400–401: expect(r2Output).toMatch(/Run with --force/i) — does not assert that entry.name, entry.path, or entry.storagePath appear in the dry-run output. remove.ts:70–73 prints all three via console.log.
  • Why it matters: The dry-run output format can silently regress (e.g. a copy-paste typo changes entry.name to entry.path) without the integration test catching it.
  • Recommendation: NIT only. The resolveRegistryEntry unit tests comprehensively cover the pure logic; the integration tests cover deletion and idempotency. Tightening the dry-run assertion would be a small improvement.
  • Confidence: High (verified from test source).

All other test coverage is solid:

  • resolveRegistryEntry: 7 unit tests covering all tiers, ambiguity, miss, empty registry, path-beats-name, case-insensitivity. ✅
  • canonicalizePath: 3 unit tests (idempotent, fallback on missing, absolute for relative). ✅
  • assertSafeStoragePath: 8 unit tests (valid, repo-root-catastrophic, parent, empty string, unrelated path, sibling .gitnexus, error payload, Windows case-insensitive). ✅
  • Backward-compat resolver: 1 unit test (stored non-canonical path matched by canonical target). ✅
  • Integration: happy-path (dry-run → force → idempotent), ambiguity-path (duplicate names → error, path-based succeeds), safety-guard (poisoned entry → exit 1, working tree survives), clean --all with poisoned entry (skips bad, cleans good). ✅

4. Performance

No issues found in this area based on the diff and reviewed context.

readRegistry() is called twice in the --force path (once in removeCommand, once inside unregisterRepo) — acknowledged in code comments as intentional under the single-writer LadybugDB model. resolveRegistryEntry is O(n) with n = registry entries; no O(n²) patterns. canonicalizePath's realpathSync.native syscall is not memoized but is called O(n) times per registry read — negligible for typical registry sizes (< 50 repos).

5. Operational risk

No issues found in this area based on the diff and reviewed context.

  • Cross-platform: canonicalizePath wires realpathSync.native with a fallback to path.resolve. All three call-sites (registerRepo, unregisterRepo, resolveRegistryEntry) canonicalize both sides consistently. No hardcoded path separators; all path operations use path.join/path.resolve. ✅
  • LadybugDB invariant: fs.rm operates on .gitnexus/ only; no direct LadybugDB API calls in the remove path. ✅
  • MCP contracts: No existing MCP tools modified. resolveRegistryEntry is exported but not yet wired — the MCP contract is additive, not a change to an existing one. ✅
  • Rollback safety: Pure code revert. No schema migration required. ✅
  • CI: Run 24714532874 completed with success (confirmed via CI API — this is the run covering the current HEAD). ✅

6. Maintainability

No issues found in this area based on the diff and reviewed context.

The change respects Pipeline Phase DAG boundaries (no ingestion code touched). resolveRegistryEntry and assertSafeStoragePath are correctly placed in repo-manager.ts alongside the other registry operations, not duplicated between CLI and server. The MCP follow-up contributor has a clear path: import resolveRegistryEntry from repo-manager.ts, reuse the same resolver, call assertSafeStoragePath before fs.rm, wire up unregisterRepo. server/api.ts is documented as safe-by-construction (it recomputes storagePath via getStoragePath(entry.path)) so no guard wiring is needed there.


Non-negotiables check

Check Status
No secrets/machine-specific paths ✅ Pass
No blind find-and-replace renames ✅ Pass (pure additions)
No HIGH/CRITICAL impact left unaddressed ✅ Pass (audited clean --all and patched it in the same PR)
PR title follows conventional commit format feat(cli): gitnexus remove <target>…
PolyForm license untouched ✅ Pass
tsc --noEmit ✅ CI typecheck passed
vitest run — all 3 platforms ✅ CI run 24714532874 = success

What I could not fully verify

  • realpathSync.native exact behavior on Windows GitHub runners at 8.3 short-name boundaries: The PR author's account is consistent with known Windows behavior, and CI passes on the Windows runner, so confidence is high — but I cannot independently reproduce the scenario.
  • canonicalizePath performance at scale: Not memoized; called O(n) times per registry read. With typical registry sizes (< 50) this is negligible; no test at pathological scale.

Final verdict

Ready to merge — no blocking issues.

The logic is correct, the safety guard closes a real and well-specified threat (corrupted storagePath → catastrophic fs.rm), the resolver contract is clean and exported for the MCP follow-up, cross-platform path handling is consistent across all call-sites, and CI is green on the current HEAD. The one NIT (weak dry-run assertion) is not merge-relevant. The implementation is production-ready for GitNexus.

…npatwari#1003 NIT)

Address the one NIT from the senior-reviewer pass on PR abhigyanpatwari#1003: the
integration test was only checking for the "Run with --force" hint in
dry-run output, not verifying that the three actual console.log lines
(alias, repo path, storage path) appear. Same weak check on the
success-branch "Removed" output.

Tighten both assertions to toContain(alias), toContain(entry.path),
toContain(storagePath). Catches silent format regressions — e.g. a
future refactor that drops a console.log line or swaps
entry.name/entry.path in the output.

No code change; +20 test lines. All assertions in the happy-path
integration test now fire for a meaningful reason.
@azizur100389

Copy link
Copy Markdown
Contributor Author

Thanks for the bot review. Addressed the one NIT in `99c47e31` — tightened the happy-path integration test to assert the full output shape of both the dry-run and the success branch (alias + resolved path + storage path all appear), not just the `Run with --force` hint. Same NIT logic covers both the dry-run and the success-case `console.log` triples, so both are now guarded against silent format regression.

Local verification on the final commit:

  • `tsc --noEmit` clean
  • `repo-manager.test.ts` 49/49
  • `cli-e2e.test.ts -t "remove|clean --all|analyze --name"` 5/5 (including tightened happy-path)
  • Full unit suite 4178 pass + 2 pre-existing git-utils env failures (unchanged, unrelated)

CI running.

@magyargergo magyargergo merged commit bd271da into abhigyanpatwari:main Apr 21, 2026
12 of 13 checks passed
@azizur100389 azizur100389 deleted the feat/cli-remove-command branch April 21, 2026 11:27
caork added a commit to caork/GitNexus that referenced this pull request Apr 22, 2026
Key upstream changes merged:
- feat(cli): gitnexus remove <target> command (abhigyanpatwari#1003)
- feat(python): scope-based call resolution pipeline RFC abhigyanpatwari#909 Ring 3 (abhigyanpatwari#980)
- feat(cli): repo fingerprinting via remote URL for sibling-clone detection (abhigyanpatwari#982)
- fix(group): friendly error on missing group name (abhigyanpatwari#989)
- fix(group): bubble local-impact errors in groupImpact (abhigyanpatwari#1007)
- fix(fts): don't cache failed FTS index (abhigyanpatwari#1006)
- fix(ci): docker alpine → debian (abhigyanpatwari#1014)
- deps: graphology 0.26.0, uuid 14, @types/node 25, @types/uuid 11

Conflict resolutions:
- AGENTS.md: kept our NEVER-format rules, added upstream language-specific
  hook rule, Tools Quick Reference table, Impact Risk Levels, gRPC guide link
- package.json: kept @types/multer, took upstream node/uuid type bumps
- package-lock.json: regenerated via npm install

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
github714801013 pushed a commit to github714801013/GitNexus that referenced this pull request Apr 28, 2026
…ame or path (abhigyanpatwari#664) (abhigyanpatwari#1003)

* feat(cli): gitnexus remove <target> to unindex a registered repo by name or path (abhigyanpatwari#664)

Add a `remove` CLI command that deletes the `.gitnexus/` index AND
unregisters a repo from the global registry (~/.gitnexus/registry.json),
addressing the lifecycle gap flagged in abhigyanpatwari#664: previously users had to
cd into the repo to run `clean`, and there was no path-based or
alias-based remove for an already-deleted working tree.

- New command `gitnexus remove <target> [-f|--force]`. `<target>` is
  alias / basename-derived name / remote-inferred name / absolute path.
- New helper `resolveRegistryEntry(entries, target)` in repo-manager.ts
  with path > name precedence; throws RegistryNotFoundError or
  RegistryAmbiguousTargetError (typed, `kind`-discriminated).
- Atomicity mirrors `clean`: fs.rm first, then unregisterRepo; partial
  failures self-heal on next `listRegisteredRepos({ validate: true })`.
- Idempotent on unknown targets (exit 0 with warning) per the abhigyanpatwari#664
  spec: "behave atomically and idempotently so retries are safe".
- `--force` uses `clean`-style confirmation-skip semantics — distinct
  from `analyze --force` (pipeline re-index); here there is no pipeline
  so no conflation.
- 7 new unit tests cover resolver precedence, case sensitivity,
  ambiguity, and not-found hints; 2 integration tests cover the real
  CLI -> registry -> filesystem chain including the --allow-duplicate-name
  (abhigyanpatwari#829) ambiguity case.

* fix(cli): canonicalize repo paths so remove/register match across platforms (abhigyanpatwari#1003 review)

Address review feedback from @evander-wang and @magyargergo on PR abhigyanpatwari#1003
plus the Windows + macOS CI failure (same root cause).

Problem:
- macOS: /var is a symlink to /private/var. `path.resolve` does NOT
  follow symlinks, so a child running analyze in /var/folders/X stores
  /private/var/folders/X (realpath from OS cwd) but an outer caller
  passing the symlink form misses.
- Windows: GitHub runners surface tmpdirs in 8.3 short-name form
  (RUNNERA~1) while process.cwd() returns the long form (runneradmin).
  Same divergence.

Fix: new `canonicalizePath(p)` helper wraps `path.resolve` plus
`fs.realpathSync.native`, falling back to `path.resolve` when the path
doesn't exist (preserves idempotent-on-missing semantics needed by
`remove <unknown>`). Applied at 3 call-sites — registerRepo,
unregisterRepo, resolveRegistryEntry — canonicalising BOTH the input
and each stored `entry.path` at compare time. That last bit is the
backward-compat story: registries written by older versions
(pre-canonicalisation) still match correctly, so we don't need a
migration script.

Test side: the ambiguous-target integration test now reads the path
from the registry snapshot rather than passing the outer `repoA`
variable directly, so it exercises the registry contract regardless of
which path form the platform stores. 4 new unit tests cover the helper
(idempotent, fallback-on-missing, absolute-for-relative) plus the
backward-compat resolver path.

* fix(cli): store resolved (non-canonical) path, compare via canonicalizePath (abhigyanpatwari#1003 CI)

Follow-up to c5eceba. The previous commit canonicalised the repo path
at BOTH write-time AND compare-time in registerRepo — that expanded
Windows 8.3 short names (RUNNER~1) to long names (runneradmin) when
storing `entry.path`. Pre-existing abhigyanpatwari#829 unit tests that assert
`path.resolve(err.existingPath) === path.resolve(tmpPath)` then broke
because `tmpPath` is still short-form (path.resolve doesn't expand
8.3) while `entry.path` was long-form (canonicalizePath does).

Fix: split storage from comparison.
- entry.path stores `path.resolve(repoPath)` — whatever form the
  caller passed. `list` output and error messages show the path the
  user typed.
- All compare points (existing-entry lookup in registerRepo, the
  collision guard, unregisterRepo, resolveRegistryEntry path tier)
  canonicalise BOTH sides via `canonicalizePath`. That is where the
  /var ↔ /private/var and RUNNER~1 ↔ runneradmin divergence actually
  matters.

Net effect: storage is tolerant (preserves user input), matching is
strict (canonical-vs-canonical). Pre-existing abhigyanpatwari#829 tests stay green
because `err.existingPath` is unchanged from what `path.resolve` gives
back; the cross-platform CI failure from abhigyanpatwari#1003 stays fixed because
every comparison path goes through `canonicalizePath`.

* fix(cli): refuse destructive fs.rm when registry storagePath isn't <repo>/.gitnexus (abhigyanpatwari#1003 review)

Address @magyargergo's inline review finding on remove.ts:89 and the
sibling vulnerability in clean.ts --all (caught during a pre-commit
safety audit). ~/.gitnexus/registry.json is a user-writable plain-text
file, so a corrupted or hand-edited entry could point storagePath at
the repo root (catastrophic: rm the working tree), an empty string
(→ cwd), a parent dir, or anywhere else. fs.rm(recursive: true,
force: true) on any of those is a runtime disaster.

- New UnsafeStoragePathError + exported assertSafeStoragePath() in
  repo-manager.ts. Pure lexical string check (Windows-case-
  insensitive) asserting entry.storagePath === path.join(entry.path,
  '.gitnexus').
- Guard wired into BOTH destructive registry-trusting sites:
  - remove.ts: exit 1 with actionable hint
  - clean.ts --all: skip the poisoned entry with a warning and
    continue (preserves existing per-repo error tolerance — one bad
    entry doesn't halt the batch)
- clean.ts default path and server/api.ts are safe-by-construction
  (they recompute storagePath from findRepo / getStoragePath rather
  than trusting the registry field).
- 8 unit tests cover the guard (valid, repo-root, parent, empty,
  unrelated, sibling, error payload, Windows case).
- 2 integration tests prove the full CLI path: remove-poisoned exits
  1 without touching the working tree; clean --all with a poisoned
  sibling entry cleans the good entry, skips the bad one, and leaves
  the poisoned repo intact.

* test(cli): assert full remove dry-run + success output shape (abhigyanpatwari#1003 NIT)

Address the one NIT from the senior-reviewer pass on PR abhigyanpatwari#1003: the
integration test was only checking for the "Run with --force" hint in
dry-run output, not verifying that the three actual console.log lines
(alias, repo path, storage path) appear. Same weak check on the
success-branch "Removed" output.

Tighten both assertions to toContain(alias), toContain(entry.path),
toContain(storagePath). Catches silent format regressions — e.g. a
future refactor that drops a console.log line or swaps
entry.name/entry.path in the output.

No code change; +20 test lines. All assertions in the happy-path
integration test now fire for a meaningful reason.
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.

Add a feature to remove a indexed repo

3 participants