Refactor skill installation to setup with dynamic discovery#4
Closed
L1nusB wants to merge 15 commits into
Closed
Conversation
Design and implement tests before the actual refactor to establish acceptance criteria and regression guards. - Export `installSkillsTo` and `SKILL_NAMES` from setup.ts for testability - Add regression tests to ai-context.test.ts (dynamic stats, idempotency, content preservation) + todo placeholders for post-refactor behavior - New setup-skills.test.ts: validates installSkillsTo installs all 6 skills correctly, with idempotency + todo placeholders for cleanup logic - New analyze-skills-notice.test.ts: tests stale project-local skills deprecation notice (warn-only, no deletion) - Update PR tracking doc with progress log All 852 unit tests pass, no regressions.
Turn skill-installation refactor acceptance tests from todo placeholders into active checks. Harden weak assertions and isolate temp dirs per test. Document the test-phase changelog and expected red-state before implementation.
- Remove installSkills() from ai-context.ts — generateAIContextFiles now only produces CLAUDE.md and AGENTS.md with dynamic index stats - Clean up unused fileURLToPath/__dirname imports from ai-context.ts - Export checkStaleProjectSkills() from analyze.ts that warns users about stale project-local skills without deleting them - Add cleanupProjectLocalSkills() to setup.ts that removes .claude/skills/gitnexus/ when running in a git repo, since skills are now installed globally - Update analyze setup tip to mention skills alongside MCP Skills are static assets that don't depend on the repository index. Installing them on every analyze run was unnecessary and caused duplicate entries in Claude Code when both setup and analyze had been run (known Claude Code bug #25209). Setup is now the single owner of skill installation. All 859 unit tests pass.
Resolve project-local skill cleanup from nested cwd by locating repo root via .git marker file/dir. Emit stale-skill migration notice even when analyze exits early as already up to date. Add regression tests and refresh refactor docs/status after Phase 3b fixes.
- Fix skill count: 4 → 6, add Guide and CLI to the list - Clarify that analyze indexes + creates context files, while setup handles MCP config, skills, and hooks - Update skill install location from .claude/skills/ to ~/.claude/skills/ - Update CLI command descriptions
Replace the "future work" section with a concrete Phase 5 plan to auto-discover skill names from the skills/ source directory instead of hardcoding them. Add 7 new test cases (abhigyanpatwari#27-33) to the test plan covering discovery logic, prefix filtering, and mixed layouts.
…33) 8 tests covering auto-discovery of skill names from the skills/ source directory: real source smoke test, prefix filtering, flat file discovery, directory-based discovery, mixed layouts, empty dir, and directories without SKILL.md. All 8 fail as expected — discoverSkillNames() is not yet exported from setup.ts. Existing 13 setup-skills tests remain green.
Replace hardcoded SKILL_NAMES array with discoverSkillNames() that reads the skills/ source directory at install time. This: - Automatically picks up gitnexus-pr-review (7 skills, up from 6) - Prevents breakage when skill files are renamed or added - Uses filesystem as source of truth instead of a hardcoded list Discovery supports flat files (*.md) and directory-based skills (*/SKILL.md), filtered to gitnexus-* prefix only. SKILL_NAMES export preserved (lazily populated) for backward compatibility. 870/870 unit tests passing.
WalkthroughRefactors skill installation from analyze to setup with dynamic skill discovery and project-local cleanup; removes installSkills from ai-context; adds stale-project-skill checks in analyze; updates docs and SKILL.md formatting; expands README and test coverage for the refactor. Changes
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
coderabbitai
Bot
changed the title
Cover duplicate-name collision, missing root and EACCES propagation, and lazy SKILL_NAMES population contract. Update refactor docs/test plan with new case IDs (abhigyanpatwari#34-abhigyanpatwari#37) and updated passing totals.
![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)
gitnexus/src/cli/setup.ts (1)
300-335:⚠️ Potential issue | 🟠 MajorScope the skip logic to missing sources only.
The per-skill
catchcurrently swallows every failure in the copy/write path, soEACCES, disk-full writes, orcopyDirRecursive()failures become silent skips instead of surfacing throughsetup. That can leave users with a partial global install and no error message.Possible fix
for (const skillName of skillNames) { const skillDir = path.join(targetDir, skillName); - - try { - // Try directory-based skill first (skills/{name}/SKILL.md) - const dirSource = path.join(skillsRoot, skillName); - const dirSkillFile = path.join(dirSource, 'SKILL.md'); - - let isDirectory = false; - try { - const stat = await fs.stat(dirSource); - isDirectory = stat.isDirectory(); - } catch { /* not a directory */ } - - if (isDirectory) { - await copyDirRecursive(dirSource, skillDir); - installed.push(skillName); - } else { - // Fall back to flat file (skills/{name}.md) - const flatSource = path.join(skillsRoot, `${skillName}.md`); - const content = await fs.readFile(flatSource, 'utf-8'); - await fs.mkdir(skillDir, { recursive: true }); - await fs.writeFile(path.join(skillDir, 'SKILL.md'), content, 'utf-8'); - installed.push(skillName); - } - } catch { - // Source skill not found — skip - } + const dirSource = path.join(skillsRoot, skillName); + + let isDirectory = false; + try { + const stat = await fs.stat(dirSource); + isDirectory = stat.isDirectory(); + } catch (err: any) { + if (err?.code !== 'ENOENT') throw err; + } + + if (isDirectory) { + await copyDirRecursive(dirSource, skillDir); + installed.push(skillName); + continue; + } + + const flatSource = path.join(skillsRoot, `${skillName}.md`); + let content: string; + try { + content = await fs.readFile(flatSource, 'utf-8'); + } catch (err: any) { + if (err?.code === 'ENOENT') continue; + throw err; + } + + await fs.mkdir(skillDir, { recursive: true }); + await fs.writeFile(path.join(skillDir, 'SKILL.md'), content, 'utf-8'); + installed.push(skillName); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@gitnexus/src/cli/setup.ts` around lines 300 - 335, The catch in installSkillsTo currently swallows all errors and hides real failures; change the logic to first check existence of dirSource and flatSource (using fs.stat or fs.access) and only skip when the source is missing (ENOENT), but let other errors from copyDirRecursive, fs.readFile, fs.mkdir, or fs.writeFile propagate (rethrow) or surface (i.e., do not catch them) so real permissions or IO failures fail the install; keep references to installSkillsTo, dirSource, flatSource, copyDirRecursive, and the fs.* calls to locate where to adjust the error handling.
🧹 Nitpick comments (1)
gitnexus/test/unit/setup-skills.test.ts (1)
75-88: Make the missing-source fixture deterministic.This mock only hits the flat-file fallback while the last discovered skill happens to be backed by
skills/{name}.md. If a directory-based skill sorts last, the test stops covering the intended branch. Pick a known flat-file skill by inspectingSKILLS_ROOT, or stub discovery to a fixed fixture list here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@gitnexus/test/unit/setup-skills.test.ts` around lines 75 - 88, The test's ENOENT mock is non-deterministic because SKILL_NAMES[last] may refer to a directory-backed skill; instead pick a known flat-file skill or stub discovery so the mocked fs.readFile always targets a file-based skill: locate a flat-file name by checking SKILLS_ROOT for an existing `${name}.md` (using SKILL_NAMES.find(...) against SKILLS_ROOT) or replace the discovery result passed into installSkillsTo with a fixed list containing a known file-backed skill, then apply the fs.readFile mock for that chosen skill (references: SKILL_NAMES, SKILLS_ROOT, installSkillsTo, fs.readFile/readFileSpy).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@gitnexus/src/cli/setup.ts`:
- Around line 421-438: Only remove repo-local Claude skills after a successful
global install: add a boolean parameter (e.g. globalInstallSucceeded) to
cleanupProjectLocalSkills and early-return if it's false, so the deletion
(fs.rm) runs only when the global install succeeded; also stop logging the
removal as a configured install (remove the push to result.configured) and
instead record it under a neutral/cleanup field (e.g. result.cleaned or
result.actions — create the field on SetupResult if missing) so summary logic
won’t treat it as a newly installed skill; apply the same change for the other
cleanup call mentioned (the second occurrence referenced in the review).
In `@README.md`:
- Around line 185-192: The README still says "6 agent skills" and lists six
items; update that header and list to reflect the auto-discovery addition by
changing the count to "7 agent skills" and include the missing skill
"gitnexus-pr-review" in the bullets (or otherwise ensure the list matches the
seven-skill total referenced in docs/pr-skill-installation-refactor.md);
specifically modify the "6 agent skills" string and add the "gitnexus-pr-review"
entry so the README and the auto-discovery documentation are consistent.
---
Outside diff comments:
In `@gitnexus/src/cli/setup.ts`:
- Around line 300-335: The catch in installSkillsTo currently swallows all
errors and hides real failures; change the logic to first check existence of
dirSource and flatSource (using fs.stat or fs.access) and only skip when the
source is missing (ENOENT), but let other errors from copyDirRecursive,
fs.readFile, fs.mkdir, or fs.writeFile propagate (rethrow) or surface (i.e., do
not catch them) so real permissions or IO failures fail the install; keep
references to installSkillsTo, dirSource, flatSource, copyDirRecursive, and the
fs.* calls to locate where to adjust the error handling.
---
Nitpick comments:
In `@gitnexus/test/unit/setup-skills.test.ts`:
- Around line 75-88: The test's ENOENT mock is non-deterministic because
SKILL_NAMES[last] may refer to a directory-backed skill; instead pick a known
flat-file skill or stub discovery so the mocked fs.readFile always targets a
file-based skill: locate a flat-file name by checking SKILLS_ROOT for an
existing `${name}.md` (using SKILL_NAMES.find(...) against SKILLS_ROOT) or
replace the discovery result passed into installSkillsTo with a fixed list
containing a known file-backed skill, then apply the fs.readFile mock for that
chosen skill (references: SKILL_NAMES, SKILLS_ROOT, installSkillsTo,
fs.readFile/readFileSpy).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: caa874cd-7cc7-4e27-9e1b-bcf61aa3daf6
⛔ Files ignored due to path filters (1)
gitnexus/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (14)
.claude/skills/gitnexus/gitnexus-cli/SKILL.md.claude/skills/gitnexus/gitnexus-guide/SKILL.mdAGENTS.mdCLAUDE.mdREADME.mddocs/pr-description.mddocs/pr-skill-installation-refactor.mddocs/test-plan-skill-installation-refactor.mdgitnexus/src/cli/ai-context.tsgitnexus/src/cli/analyze.tsgitnexus/src/cli/setup.tsgitnexus/test/unit/ai-context.test.tsgitnexus/test/unit/analyze-skills-notice.test.tsgitnexus/test/unit/setup-skills.test.ts
💤 Files with no reviewable changes (1)
- gitnexus/src/cli/ai-context.ts
- Guard cleanupProjectLocalSkills on successful global install: don't remove local skills if no global replacement was actually installed - Update README skill count from 6 to 7, add gitnexus-pr-review entry - Stop swallowing non-ENOENT errors in installSkillsTo — only skip missing source skills, propagate real I/O failures 874/874 unit tests passing.
There was a problem hiding this comment.
🧹 Nitpick comments (3)
docs/pr-skill-installation-refactor.md (1)
196-199: Residual edge case is now resolved in code.Line 198 notes that "setup cleanup runs regardless of whether any global skill install actually succeeded." This concern has been addressed in the implementation — the guard at
setup.ts:474-477now checksglobalSkillsInstalledbefore running cleanup.Consider updating this note to reflect the fix.
📝 Proposed update
Residual edge cases found in review (not addressed in this branch): - The updated `analyze` setup tip is effectively unreachable after successful indexing because `registerRepo()` creates the global registry before the tip check. -- `setup` cleanup runs regardless of whether any global skill install actually succeeded; on machines with no supported editor directories present, stale local skills could be removed without replacement. +- ~~`setup` cleanup runs regardless of whether any global skill install actually succeeded~~ — **Fixed**: cleanup now checks `globalSkillsInstalled` before running (Phase 3b).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/pr-skill-installation-refactor.md` around lines 196 - 199, Update the docs note to reflect that the edge case was fixed: mention that the setup cleanup now runs only when a global skill install succeeded by referencing the guard variable globalSkillsInstalled in setup.ts (the check that prevents cleanup unless globalSkillsInstalled is true) and remove or reword the bullet about "setup cleanup runs regardless..."; also adjust the first bullet to clarify that the analyze tip remains unreachable in the path where registerRepo() creates the global registry before the tip check if that behavior is unchanged.gitnexus/src/cli/setup.ts (1)
310-311: Remove unused variabledirSkillFile.The variable
dirSkillFileis assigned but never used. The directory check relies onisDirectoryinstead.🧹 Proposed fix
// Try directory-based skill first (skills/{name}/SKILL.md) const dirSource = path.join(skillsRoot, skillName); - const dirSkillFile = path.join(dirSource, 'SKILL.md'); let isDirectory = false;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@gitnexus/src/cli/setup.ts` around lines 310 - 311, The variable dirSkillFile (declared as const dirSkillFile = path.join(dirSource, 'SKILL.md')) is unused; remove its declaration to clean up the code. In the setup logic that constructs dirSource (using skillsRoot and skillName) keep dirSource and the existing isDirectory check, but delete the dirSkillFile line and any subsequent unused references; run a quick grep for dirSkillFile to ensure no other code depends on it before committing.docs/test-plan-skill-installation-refactor.md (1)
126-134: Gap#25is now resolved — consider updating.The residual coverage gap about "cleanup behavior when no global skill target is installed/configured" has been addressed in the implementation. The guard at
setup.ts:474-477now prevents cleanup when no global skills were installed. Consider updating this section or adding a test to cover this scenario.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/test-plan-skill-installation-refactor.md` around lines 126 - 134, Update the test-plan to mark Residual Coverage Gap `#25` as resolved: remove or annotate the table row for "Cleanup behavior when no global skill target is installed/configured" and add a brief note pointing to the implemented guard in setup (the conditional around the cleanup logic that prevents cleanup when no global skills were installed, referenced in setup.ts lines where the guard is added) and/or add a new test entry that verifies the guard prevents cleanup when no global target exists; ensure the docs reflect the fix and link the test suggestion to the setup cleanup guard (the conditional in the setup cleanup code).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@docs/pr-skill-installation-refactor.md`:
- Around line 196-199: Update the docs note to reflect that the edge case was
fixed: mention that the setup cleanup now runs only when a global skill install
succeeded by referencing the guard variable globalSkillsInstalled in setup.ts
(the check that prevents cleanup unless globalSkillsInstalled is true) and
remove or reword the bullet about "setup cleanup runs regardless..."; also
adjust the first bullet to clarify that the analyze tip remains unreachable in
the path where registerRepo() creates the global registry before the tip check
if that behavior is unchanged.
In `@docs/test-plan-skill-installation-refactor.md`:
- Around line 126-134: Update the test-plan to mark Residual Coverage Gap `#25` as
resolved: remove or annotate the table row for "Cleanup behavior when no global
skill target is installed/configured" and add a brief note pointing to the
implemented guard in setup (the conditional around the cleanup logic that
prevents cleanup when no global skills were installed, referenced in setup.ts
lines where the guard is added) and/or add a new test entry that verifies the
guard prevents cleanup when no global target exists; ensure the docs reflect the
fix and link the test suggestion to the setup cleanup guard (the conditional in
the setup cleanup code).
In `@gitnexus/src/cli/setup.ts`:
- Around line 310-311: The variable dirSkillFile (declared as const dirSkillFile
= path.join(dirSource, 'SKILL.md')) is unused; remove its declaration to clean
up the code. In the setup logic that constructs dirSource (using skillsRoot and
skillName) keep dirSource and the existing isDirectory check, but delete the
dirSkillFile line and any subsequent unused references; run a quick grep for
dirSkillFile to ensure no other code depends on it before committing.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e6cfebb9-7906-46d4-b80f-62d3a5777819
📒 Files selected for processing (6)
README.mddocs/pr-description.mddocs/pr-skill-installation-refactor.mddocs/test-plan-skill-installation-refactor.mdgitnexus/src/cli/setup.tsgitnexus/test/unit/setup-skills.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- docs/pr-description.md
- gitnexus/test/unit/setup-skills.test.ts
- Update PR doc: mark cleanup guard edge case as fixed, clarify remaining tip visibility note - Update test plan: mark gap abhigyanpatwari#25 as resolved with guard reference - Remove unused dirSkillFile variable in installSkillsTo
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
gitnexus/src/cli/setup.ts (1)
421-438:⚠️ Potential issue | 🟠 MajorOnly clean up repo-local Claude skills after a successful Claude global install.
globalSkillsInstalledflips to true for Cursor/OpenCode installs too, butcleanupProjectLocalSkills()deletes the Claude-specific repo-local directory. That can remove the only Claude-visible copy when~/.claude/skills/was never populated. The cleanup message is also still written toresult.configured, so the summary reports a deletion as an install.🛠️ Suggested direction
-interface SetupResult { - configured: string[]; - skipped: string[]; - errors: string[]; -} +interface SetupResult { + configured: string[]; + cleaned: string[]; + skipped: string[]; + errors: string[]; +} -async function installClaudeCodeSkills(result: SetupResult): Promise<void> { +async function installClaudeCodeSkills(result: SetupResult): Promise<boolean> { const claudeDir = path.join(os.homedir(), '.claude'); - if (!(await dirExists(claudeDir))) return; + if (!(await dirExists(claudeDir))) return false; const skillsDir = path.join(claudeDir, 'skills'); try { const installed = await installSkillsTo(skillsDir); if (installed.length > 0) { result.configured.push(`Claude Code skills (${installed.length} skills → ~/.claude/skills/)`); + return true; } } catch (err: any) { result.errors.push(`Claude Code skills: ${err.message}`); } + return false; } - result.configured.push('Removed project-local skills (now installed globally)'); + result.cleaned.push('Removed project-local skills (now installed globally)'); - await installClaudeCodeSkills(result); + const claudeSkillsInstalled = await installClaudeCodeSkills(result); await installClaudeCodeHooks(result); await installCursorSkills(result); await installOpenCodeSkills(result); - const globalSkillsInstalled = result.configured.some(c => c.includes('skills (')); - if (globalSkillsInstalled) { + if (claudeSkillsInstalled) { await cleanupProjectLocalSkills(result); }Also applies to: 470-475
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@gitnexus/src/cli/setup.ts` around lines 421 - 438, Add a guard so repo-local Claude skills are only removed after a successful Claude global install: change cleanupProjectLocalSkills(result: SetupResult) to cleanupProjectLocalSkills(result: SetupResult, claudeGlobalInstalled: boolean) and immediately return if claudeGlobalInstalled is false; update all callers (the sites invoking cleanupProjectLocalSkills, including the other occurrence mentioned) to pass the existing globalSkillsInstalled flag (or a dedicated boolean that represents a successful Claude install), and keep pushing the "Removed project-local..." message into result.configured only after fs.rm succeeds so the summary accurately reflects an actual removal.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/test-plan-skill-installation-refactor.md`:
- Around line 47-54: Update the hardcoded counts in the installSkillsTo test
table to reflect dynamic discovery instead of "6 skills" and "other 5": replace
literal counts with language like "all discovered skills" or refer to the
discovered set (e.g., include gitnexus-pr-review as part of the installed set),
and update test rows for installSkillsTo, SKILL.md assertions, and the
idempotency/missing-source cases so they assert against the discovered skill
list rather than fixed numbers; ensure references to installSkillsTo, SKILL.md,
and gitnexus-pr-review are updated accordingly.
In `@gitnexus/src/cli/setup.ts`:
- Around line 253-270: discoverSkillNames collects names from both gitnexus-x.md
files and gitnexus-x/SKILL.md directories and may emit duplicates (e.g.,
"gitnexus-x" twice); change the implementation in discoverSkillNames to
deduplicate the collected names (use a Set or similar) before sorting and
returning so installSkillsTo() won't process the same skill twice—ensure you
still check entry.isFile()/entry.isDirectory(), access SKILL.md, and then add to
a de-duplicated collection keyed by entry.name.
---
Duplicate comments:
In `@gitnexus/src/cli/setup.ts`:
- Around line 421-438: Add a guard so repo-local Claude skills are only removed
after a successful Claude global install: change
cleanupProjectLocalSkills(result: SetupResult) to
cleanupProjectLocalSkills(result: SetupResult, claudeGlobalInstalled: boolean)
and immediately return if claudeGlobalInstalled is false; update all callers
(the sites invoking cleanupProjectLocalSkills, including the other occurrence
mentioned) to pass the existing globalSkillsInstalled flag (or a dedicated
boolean that represents a successful Claude install), and keep pushing the
"Removed project-local..." message into result.configured only after fs.rm
succeeds so the summary accurately reflects an actual removal.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ef8b0580-f5fc-48b0-a638-1c2159e52c2e
📒 Files selected for processing (3)
docs/pr-skill-installation-refactor.mddocs/test-plan-skill-installation-refactor.mdgitnexus/src/cli/setup.ts
Comment on lines
+47
to
+54
| ### `installSkillsTo` — core skill installation | ||
|
|
||
| | # | Test | Setup | Assertion | | ||
| |---|------|-------|-----------| | ||
| | 7 | Installs all 6 skills to target directory | Call with temp dir as target | Each of the 6 skill directories exists with a `SKILL.md` file | | ||
| | 8 | Each SKILL.md has non-empty content | Call with temp dir | Every `SKILL.md` has length > 0 | | ||
| | 9 | Idempotent — re-running overwrites cleanly | Call twice | Same 6 skills, no duplicates, no errors | | ||
| | 10 | Handles missing source skill gracefully | Mock one skill file as missing | Other 5 install successfully, missing one is skipped | |
There was a problem hiding this comment.
Update the core install counts to match dynamic discovery.
This section still says “6 skills” and “other 5 install successfully,” but the later discovery section and current status now treat gitnexus-pr-review as part of the installed set. These hardcoded counts are stale and can mislead anyone using this as the current acceptance criteria.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/test-plan-skill-installation-refactor.md` around lines 47 - 54, Update
the hardcoded counts in the installSkillsTo test table to reflect dynamic
discovery instead of "6 skills" and "other 5": replace literal counts with
language like "all discovered skills" or refer to the discovered set (e.g.,
include gitnexus-pr-review as part of the installed set), and update test rows
for installSkillsTo, SKILL.md assertions, and the idempotency/missing-source
cases so they assert against the discovered skill list rather than fixed
numbers; ensure references to installSkillsTo, SKILL.md, and gitnexus-pr-review
are updated accordingly.
Comment on lines
+253
to
+270
| export async function discoverSkillNames(skillsRoot: string): Promise<string[]> { | ||
| const entries = await fs.readdir(skillsRoot, { withFileTypes: true }); | ||
| const names: string[] = []; | ||
|
|
||
| for (const entry of entries) { | ||
| if (!entry.name.startsWith('gitnexus-')) continue; | ||
|
|
||
| if (entry.isFile() && entry.name.endsWith('.md')) { | ||
| names.push(entry.name.replace(/\.md$/, '')); | ||
| } else if (entry.isDirectory()) { | ||
| try { | ||
| await fs.access(path.join(skillsRoot, entry.name, 'SKILL.md')); | ||
| names.push(entry.name); | ||
| } catch { /* no SKILL.md — skip */ } | ||
| } | ||
| } | ||
|
|
||
| return names.sort(); |
There was a problem hiding this comment.
De-duplicate discovered skill names before returning them.
If both gitnexus-x.md and gitnexus-x/SKILL.md exist, this returns gitnexus-x twice. installSkillsTo() will then copy the same target twice and over-report the installed skill count.
💡 Proposed fix
export async function discoverSkillNames(skillsRoot: string): Promise<string[]> {
const entries = await fs.readdir(skillsRoot, { withFileTypes: true });
- const names: string[] = [];
+ const names = new Set<string>();
for (const entry of entries) {
if (!entry.name.startsWith('gitnexus-')) continue;
if (entry.isFile() && entry.name.endsWith('.md')) {
- names.push(entry.name.replace(/\.md$/, ''));
+ names.add(entry.name.replace(/\.md$/, ''));
} else if (entry.isDirectory()) {
try {
await fs.access(path.join(skillsRoot, entry.name, 'SKILL.md'));
- names.push(entry.name);
+ names.add(entry.name);
} catch { /* no SKILL.md — skip */ }
}
}
- return names.sort();
+ return [...names].sort();
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@gitnexus/src/cli/setup.ts` around lines 253 - 270, discoverSkillNames
collects names from both gitnexus-x.md files and gitnexus-x/SKILL.md directories
and may emit duplicates (e.g., "gitnexus-x" twice); change the implementation in
discoverSkillNames to deduplicate the collected names (use a Set or similar)
before sorting and returning so installSkillsTo() won't process the same skill
twice—ensure you still check entry.isFile()/entry.isDirectory(), access
SKILL.md, and then add to a de-duplicated collection keyed by entry.name.
L1nusB
pushed a commit
that referenced
this pull request
Mar 30, 2026
…gyanpatwari#498) * feat: add COBOL language support with regex extraction pipeline Standalone COBOL processor following the markdown-processor.ts pattern: - No LanguageProvider modification — COBOL uses regex, not tree-sitter - No SupportedLanguages enum change — standalone processor pattern New files: - cobol-processor.ts — orchestrator (processCobol, isCobolFile, isJclFile) - cobol/cobol-preprocessor.ts — regex state machine extraction (~888 LOC) - cobol/cobol-copy-expander.ts — COPY statement expansion with circular detection - cobol/jcl-parser.ts — JCL job/step/DD extraction - cobol/jcl-processor.ts — JCL graph node creation Extraction produces: - Module nodes (PROGRAM-ID) - Function nodes (paragraphs) - Namespace nodes (sections) - Property nodes (data items) - CALLS edges (PERFORM intra-file, CALL cross-program) - IMPORTS edges (COPY statements) - CONTAINS edges (section → paragraph hierarchy) Pipeline integration: single processCobol() call in Phase 2.6 54 new tests (33 COBOL + 21 JCL), all 3889 tests pass. * docs: document custom processor pattern in pipeline.ts Add comment block at the custom processor integration point documenting the pattern for future non-tree-sitter language additions. * feat(cobol): enrich graph with EXEC SQL/CICS, ENTRY points, MOVE data flow, PERFORM THRU Maps the remaining 60% of CobolRegexResults to the graph: - EXEC SQL blocks → CodeElement nodes + ACCESSES edges to DB tables - EXEC CICS LINK/XCTL → CodeElement nodes + cross-program CALLS edges - ENTRY points → Constructor nodes (registered for cross-program resolution) - MOVE statements → ACCESSES edges (read/write data flow tracking) - PERFORM THRU → expanded CALLS edges for range targets - File declarations → Record nodes with assignment metadata - Cross-program CALL 2nd pass: resolves unresolved targets after all programs processed * test(cobol): add 26 integration tests with exact assertions + fix CICS resolution bug Integration tests (test/integration/resolvers/cobol.test.ts): - 26 tests covering full COBOL system extraction - ALL assertions use exact toBe(N) — zero fuzzy assertions - Fixtures: CUSTUPDT.cbl, AUDITLOG.cbl, CUSTDAT.cpy, RPTGEN.cbl, RUNJOBS.jcl Bug fix (cobol-processor.ts): - CICS LINK/XCTL cross-program resolution was broken — edges were created with "resolved" reason but pointing to <unresolved> targets - Fix: use cics-link-unresolved / cics-xctl-unresolved suffix pattern matching the existing cobol-call-unresolved pattern - Second-pass resolver now patches both CALL and CICS unresolved edges All 3915 tests pass, 0 failures. * test(cobol): exhaustive 57-test suite with strict exact assertions Complete rewrite of COBOL integration tests using ground-truth approach: dump the full graph, then assert EVERY node and EVERY edge. 57 tests across 9 sections: - Node completeness: Module(3), Function(13), Namespace(2), Property(21), Record(1), CodeElement(8), Constructor(1) — exact sorted arrays - Edge completeness: 22 tests covering every type+reason combination with exact source→target pairs - Cross-program resolution: 6 tests verifying CALL, CICS LINK/XCTL, JCL - COPY expansion: copybook data items in RPTGEN - Section hierarchy: exact paragraph membership per section - Data item ownership: exact per-module breakdown - MOVE data flow: exact read/write pairs - JCL integration: job/step/dataset containment - Grand totals: CALLS(22), CONTAINS(48), IMPORTS(1), ACCESSES(7) Fixture enhancements: - CUSTUPDT.cbl: added INIT-SECTION + PROCESSING-SECTION, PERFORM THRU - AUDITLOG.cbl: added ENTRY "AUDITLOG-BATCH" - RPTGEN.cbl: added EXEC CICS XCTL Zero fuzzy assertions — every expect uses toBe(N) or toEqual([...sorted]). * fix(cobol): add removeRelationship API + single-quote CALL/COPY/ENTRY, PERFORM keyword skip Phase 0A: Add removeRelationship(id) to KnowledgeGraph interface and implementation (trivial Map.delete wrapper). Required for orphan edge cleanup in next commit. Phase 1A (from PR abhigyanpatwari#500 review, modified): - RE_CALL and RE_COPY_QUOTED now match both "double" and 'single' quotes - parseSingleCopyStatement in copy-expander updated for single quotes - PERFORM_KEYWORD_SKIP set prevents UNTIL/VARYING/WITH/TEST/FOREVER from being stored as false-positive perform targets - Sequence number stripping uses /[^0-9 ]/ (preserves numeric seq numbers unlike PR abhigyanpatwari#500's /\S/ which stripped them) - Normalized || to ?? for regex group extraction in copy-expander 5 new graph unit tests, all 57 COBOL integration tests pass. * fix(cobol): RE_ENTRY single-quote + remove orphan unresolved CALLS edges Phase 1B: RE_ENTRY regex now supports both "double" and 'single' quoted ENTRY targets. Uses named intermediates (entryName, usingClause) with ?? operator. USING capture group shifted from [2] to [3]. Phase 1C: Second-pass resolution now collects resolved orphan edge IDs during iteration and removes them after the loop completes, using the new graph.removeRelationship() API. Graph no longer contains phantom <unresolved>: edges alongside their resolved replacements. CALLS count drops from 22 to 18 (4 orphan edges removed). * fix(cobol): Property ID collisions + O(1) Map lookup for MOVE edges Phase 1D+3C (atomic): Property node IDs now use composite key filePath:section:level:name instead of filePath:name. This prevents duplicate data item names in different sections (e.g., STATUS in both WORKING-STORAGE and LINKAGE) from silently colliding. New generatePropertyId() helper ensures both node creation and MOVE edge lookup use the identical key formula. buildDataItemMap() replaces the O(n) findDataItemNode linear scan with O(1) Map lookup, built once per file before MOVE processing. * feat(cobol): MOVE multi-target extraction with OF/IN qualifier filtering MOVE X TO A B C now produces write edges for all targets, not just the first. extractMoveTargets() helper handles OF/IN qualified names (WS-NAME OF WS-RECORD -> target is WS-NAME), subscript stripping (WS-TABLE(I) -> WS-TABLE), and MOVE_SKIP filtering on targets. Data model: CobolRegexResults.moves.to:string -> targets:string[] MOVE CORRESPONDING stays single-target per COBOL standard. Processor MOVE loop now iterates move.targets. * feat(cobol): COPY IN/OF library, pseudotext REPLACING, dynamic CALL, PERFORM TIMES, CICS MAP unquoted Phase 2B: COPY ... IN/OF library-name now captured as metadata in CopyResolution (IN and OF are synonyms per COBOL-85 standard). Phase 2C: COPY REPLACING ==pseudotext== support. Tokenizer handles ==...== delimiters alongside "quoted" strings. Pseudotext forces EXACT type. Two-pass applyReplacing: first pass handles space-containing/ non-identifier pseudotext via global string replace; second pass handles identifier-level LEADING/TRAILING/EXACT. New test file cobol-copy-expander.test.ts with 10 tests. Phase 2E: PERFORM WS-COUNT TIMES no longer produces a false-positive perform target (checks for TIMES keyword after captured identifier). Phase 2F: Dynamic CALL via data item (CALL WS-PROG-NAME without quotes) now emits a CodeElement annotation node with description 'dynamic-call' instead of silently ignoring. Adds isQuoted:boolean to call results. Phase 3A: CICS MAP(WS-MAP-NAME) unquoted identifiers now captured. Phase 3B: Normalized || to ?? in copy-expander (done in Phase 1A). * feat(cobol): nested program support — capture multiple PROGRAM-IDs per file Phase 2D: The state machine now captures all PROGRAM-IDs, not just the first. The primary program name stays in programName; additional nested programs go into nestedPrograms[]. The processor creates separate Module nodes for each nested program, contained by the outer module, and registers them in moduleNodeIds for cross-program CALL resolution. Paragraphs/data items are not yet scoped per-program (attributed to the outer module) — full per-program scoping is a future enhancement that requires END PROGRAM boundary tracking in the state machine. * test(cobol): expand integration tests for all new language features New fixtures: - NESTED.cbl — two PROGRAM-IDs (OUTER-PROG, INNER-PROG) for nested program support testing - COPYLIB.cpy — copybook for pseudotext REPLACING test target Modified fixtures: - CUSTUPDT.cbl — single-quoted ENTRY 'ALTENTRY', multi-target MOVE (WS-AMT TO FIELD-A FIELD-B), dynamic CALL WS-PROG-NAME, COPY COPYLIB with pseudotext REPLACING, LINKAGE SECTION with LS-PARAM - RPTGEN.cbl — PERFORM WS-COUNT TIMES (false-positive guard), unquoted MAP(WS-MAP-NAME), additional data items WS-COUNT WS-MAP-NAME Integration test rewritten with 62 exact assertions covering: - 5 Module, 17 Function, 33 Property, 9 CodeElement, 2 Constructor nodes - Nested program containment (OUTER-PROG -> INNER-PROG) - Dynamic CALL annotation (CodeElement with cobol-dynamic-call) - Multi-target MOVE (UPDATE-BALANCE: 2 reads, 3 writes) - Single-quoted ENTRY (ALTENTRY under CUSTUPDT) - PERFORM TIMES guard (WS-COUNT not in CALLS) - Orphan unresolved edge removal (zero -unresolved edges) - Grand totals: 21 CALLS, 68 CONTAINS, 2 IMPORTS, 10 ACCESSES * fix(cobol): pseudotext REPLACING now applies correctly via isPseudotext flag Root cause: ==PREFIX-== matched /^[A-Z][A-Z0-9-]*$/i (trailing hyphens allowed), routing it to the second-pass EXACT identifier match where PREFIX-RECORD !== PREFIX- failed silently. Fix: Propagate isPseudotext from parseReplacingClause to CopyReplacing interface, then use it in applyReplacing first-pass condition to force global string replacement for all pseudotext entries regardless of whether the content looks like an identifier. Result: COPY COPYLIB REPLACING ==PREFIX-== BY ==WS-==. now correctly transforms PREFIX-RECORD → WS-RECORD, PREFIX-CODE → WS-CODE, etc. * refactor(cobol): per-program scoping via boundary tracking + line-range grouping State machine changes (minimal, ~30 lines): - Add RE_END_PROGRAM regex for END PROGRAM program-name. detection - Replace nestedPrograms[] with programs[] containing startLine/endLine/ nestingDepth metadata for each PROGRAM-ID in the file - Reset division/section/paragraph state on new PROGRAM-ID boundary - EOF finalization flushes remaining stack entries (single-program files) - Programs sorted by startLine (outer before inner) Processor changes: - Uses programs[] with line-range containment to find enclosing parent Module for nested programs (replaces hardcoded nestedParent logic) - programModuleIds Map tracks Module node IDs per program name Fixture: NESTED.cbl now includes END PROGRAM lines for both programs. Integration test: PREFIX-* Property nodes now correctly appear as WS-* after the pseudotext REPLACING fix from the previous commit. * feat(cobol): free-format COBOL support (>>source free) Auto-detects >>SOURCE FREE directive in the first 500 chars and switches to free-format line processing: - No column-position rules (cols 1-6 are program text, not sequence area) - Comments use *> prefix instead of col 7 indicator - No continuation line indicator - Strip inline *> comments - Skip >>SOURCE directive lines preprocessCobolSource() skips col-1-6 stripping for free-format files. Paragraph/section regexes relaxed from fixed 7-space prefix to flexible whitespace with case-insensitivity (/^\s*([A-Z][A-Z0-9-]+)\.\s*$/i). EXCLUDED_PARA_NAMES expanded with COBOL verbs (GOBACK, END-READ, etc.) to prevent false-positive paragraph detection in free-format. Also fixes: entry-point-scoring.ts crash when language is 'cobol' (MERGED_ENTRY_POINT_PATTERNS[language] was undefined → optional chaining). Benchmark on ACAS 3.01 (268 GnuCOBOL free-format programs, 10MB): - Before: 407 nodes, 393 edges (near-empty, only file nodes) - After: 4,297 nodes, 3,612 edges, 542 clusters, 11 flows * fix(cobol): relax data item regexes for free-format (^\s+ to ^\s*) RE_FD, RE_DATA_ITEM, RE_ANONYMOUS_REDEFINES, and RE_88_LEVEL all used ^\s+ which requires at least 1 leading space. In free-format mode, lines are trimmed before processing, so data items like "01 WS-FIELD PIC X." have no leading whitespace after trimming. Changed to ^\s* (zero or more spaces) which works for both fixed-format (indented lines still have spaces) and free-format (trimmed lines). ACAS benchmark (268 GnuCOBOL programs): - Before: 4,297 nodes, 3,612 edges (paragraphs only) - After: 13,832 nodes, 8,615 edges (+ data items, FDs, 88-levels) * feat(cobol): 100% structural feature coverage — GO TO, SCREEN, SD/RD, SORT, SEARCH, CANCEL, Level 66 New extractions: GO TO (CALLS edges), SCREEN SECTION data items, SD/RD alongside FD (Record nodes), SORT/MERGE USING/GIVING (ACCESSES), SEARCH (ACCESSES), CANCEL (CALLS), Level 66 RENAMES (Property), IS EXTERNAL/IS GLOBAL (Property description enrichment). ACAS: 13,951 nodes | 13,193 edges | 685 clusters | 150 flows (+53% edges from new GO TO/SORT/SEARCH/CANCEL extractions) * feat(cobol): enriched CICS extraction — file I/O, dynamic PROGRAM, queues, HANDLE ABEND EXEC CICS blocks now extract: - FILE/DATASET clause: captures VSAM file name (literal or data item ref) for READ/WRITE/REWRITE/DELETE/STARTBR/READNEXT/READPREV → ACCESSES edges - PROGRAM clause: now handles unquoted variable references (dynamic CICS program transfer) → CodeElement annotation with cics-dynamic-program reason - QUEUE clause: captures TS/TD queue names from WRITEQ/READQ → ACCESSES edges - LABEL clause: captures HANDLE ABEND error handler targets → CALLS edges - TRANSID: now handles unquoted variable references CodeElement descriptions enriched with all captured fields (map, program, transid, file, queue, label). CardDemo benchmark: +49 nodes, +33 edges from enriched CICS extraction. * feat(cobol): complete CICS command extraction — all 7 expert recommendations From COBOL expert agent analysis: 1. ENDBR added to isRead file command list 2. LOAD added to PROGRAM edge commands (alongside LINK/XCTL) 3. Two-word commands expanded: WRITEQ/READQ/DELETEQ TS/TD, HANDLE ABEND/AID/CONDITION, START TRANSID 4. Queue reason differentiated: cics-queue-read/-write/-delete 5. RETURN/START TRANSID → CALLS edges to synthetic <transid> target 6. MAP → ACCESSES edges for screen traceability 7. INTO/FROM data fields extracted → ACCESSES edges to data items Also: dataItemMap built before CICS block processing (was declared after), CodeElement descriptions enriched with all captured CICS fields. * test(cobol): strict exhaustive integration tests with exact edgeSet assertions Every edge reason has exact sorted pair assertions via edgeSet(), not just counts. Any change to extraction that adds, removes, or reorders edges will produce a precise, descriptive failure. Updated RPTGEN.cbl fixture with: - GO TO EXIT-PARAGRAPH, SORT USING/GIVING, SEARCH table - EXEC CICS READ FILE INTO, WRITEQ TS QUEUE FROM, SEND MAP FROM - EXEC CICS HANDLE ABEND LABEL, RETURN TRANSID, XCTL PROGRAM(variable) - ABEND-HANDLER and EXIT-PARAGRAPH paragraphs 46 tests covering 24 CALLS + 79 CONTAINS + 18 ACCESSES + 2 IMPORTS edges across 15 distinct edge reason codes, all with exact sorted pair lists. * fix(cobol): address 5 findings from second Claude review (compiler front-end perspective) Finding #2: Numeric sequence numbers now stripped (changed /[^0-9 ]/ to /\S/ in preprocessCobolSource). Lines like "000100 MAIN-PARAGRAPH." now have cols 1-6 blanked so paragraph regex matches correctly. Finding abhigyanpatwari#11: JCL in-stream PROC ordering fixed — pre-register all PROCs into moduleNames before step processing. Steps that EXEC a PROC defined later in the same file now get CALLS edges. Finding #A: PROCEDURE DIVISION USING no longer captures calling-convention keywords (BY, VALUE, REFERENCE, CONTENT, ADDRESS, OF) as parameter names. Finding #C: SORT/MERGE USING/GIVING now captures ALL file references (multi-file), not just the first. Changed from single-match to section extraction with split. Finding #D: Section headers no longer set currentParagraph, preventing PERFORM caller misattribution to Namespace instead of Function nodes. * fix(cobol): address code review findings — ReDoS fix, perf, cleanup P1 CRITICAL — ReDoS in SORT USING/GIVING: Replaced nested-quantifier regex with safe indexOf+substring+split approach. No backtracking possible on crafted input. P2 — readCopy O(M) linear scan: Added copybookByPath reverse Map for O(1) path-to-content lookup. P3 — Dead code removal: Deleted unused RE_SORT_USING and RE_SORT_GIVING constants. P3 — EXCLUDED_PARA_NAMES simplification: Replaced 20 END-* entries with startsWith('END-') prefix check. Auto-covers future END-* verbs. P3 — Misplaced JSDoc on removeRelationship: Fixed comment that described removeNodesByFile instead. Added missing JSDoc to removeNodesByFile. Review agents: architecture-strategist, performance-oracle, security-sentinel, code-simplicity-reviewer. * refactor: add Cobol to SupportedLanguages with parseStrategy: standalone New languages/cobol.ts — standalone regex processor provider with no-op tree-sitter fields. Declares parseStrategy: 'standalone' to distinguish from tree-sitter-based languages. Added parseStrategy: 'tree-sitter' | 'standalone' to LanguageProviderConfig for languages that use their own processor instead of tree-sitter. Removed all 11 'cobol' as any casts — now uses SupportedLanguages.Cobol. Added empty Cobol entries to entry-point-scoring and framework-detection. * fix(cobol): 5 fixes from third Claude review + 3 regression tests Fixes: - Line numbers now 1-indexed in fixed-format (was 0-indexed, off-by-one in jump-to-definition links) - Copybook content preprocessed before COPY expansion (sequence numbers and patch markers in copybooks no longer survive into expanded source) - ENTRY USING filters calling-convention keywords (BY, VALUE, REFERENCE, CONTENT, ADDRESS, OF) — same fix as PROCEDURE DIVISION USING - SORT/MERGE trailing period stripped from USING/GIVING file tokens - Paragraph exclusion uses exact match for SECTION/DIVISION (was substring match that excluded valid names like CROSS-SECTION-ANALYSIS) USING_KEYWORDS moved to module scope for reuse by both PROCEDURE DIVISION USING and ENTRY USING handlers. New unit tests: - ENTRY USING BY VALUE filtering - Paragraph names containing SECTION not excluded - Numeric sequence numbers stripped enabling paragraph detection * fix(cobol): address 6 findings from fourth Claude review + tests Fourth review findings fixed: - New #IV: PERFORM TIMES guard uses perfMatch.index instead of line.indexOf (prevents wrong match when target appears earlier in line) - New #V: 88-level condition values now handle single-quoted literals ('Y' no longer stored with embedded quotes) - New #I: CANCEL edges use two-pass resolution like CALL (no longer silently dropped when target indexed after source) - New #3: Multi-line SORT/MERGE accumulation — sortAccum state variable accumulates lines until period, then extracts USING/GIVING from full statement (95% of production SORT statements span multiple lines) - New #II: PROCEDURE DIVISION USING on split lines — pendingProcUsing flag defers parameter capture to next line if USING not on same line - New #6 (prior): EXCLUDED_PARA_NAMES exact match for SECTION/DIVISION Updated fixture: RPTGEN.cbl SORT now uses multi-line format with GIVING on separate line (period-terminated). New sort-giving integration test. ACCESSES total: 18 → 19 (new sort-giving edge from multi-line capture). * fix(cobol): address 4 findings from fifth Claude review Finding #B (5 reviews old): Section/paragraph node IDs now include enclosing program name to prevent collision when nested programs share section/paragraph names. New findOwningProgramName() helper uses programs[] line ranges to find the innermost enclosing program. Finding #α: pendingProcUsing now reset in the if(procUsingMatch) branch (was only set in else branch, could leak across nested programs). Finding #β: RE_CALL_DYNAMIC uses negative lookbehind (?<![A-Z0-9-]) to prevent false-positive on compound identifiers like WS-CALL OCCURS. Finding #γ: sortAccum flushed at EOF (parallel to flushSelect and pendingFdName EOF cleanup). Prevents silent loss of SORT USING/GIVING relationships in truncated files. * fix(cobol): address findings from reviews 5+6 with full test coverage Review 5 fixes: - #α: pendingProcUsing reset in if(procUsingMatch) branch - #β: RE_CALL_DYNAMIC negative lookbehind prevents WS-CALL false positive - #γ: sortAccum flushed at EOF for truncated files - #B: Section/paragraph IDs include owning program name Review 6 fixes: - #P: sectionNodeIds/paraNodeIds maps use program-scoped keys (PROGNAME:NAME). New scopedParaLookup/scopedCallerLookup helpers. findContainingSection updated with programs parameter. - #Q: RETURNING added to USING_KEYWORDS for COBOL 2002+ - #R: RE_PERFORM matches both THRU and THROUGH via alternation New unit tests (6): - PERFORM THROUGH captures thruTarget - PROCEDURE DIVISION USING RETURNING filters keyword - RE_CALL_DYNAMIC no false-match on WS-CALL compound identifier - Multi-line SORT captures USING/GIVING from continuation lines - PROCEDURE DIVISION USING on split line via pendingProcUsing - Copybook preprocessing strips sequence numbers * fix(cobol): address findings from seventh Claude review + 3 tests Review 7 fixes: - #i: findContainingSection only updates best when lookup succeeds (prevents undefined overwriting valid parent section) - #ii: RE_PROC_SECTION handles segment numbers (SECTION 30.) - #III: procedureUsing now stored per-program on boundary stack entries, propagated to programs[] output. Inner programs no longer overwrite outer program's parameters. - #δ: Dynamic CANCEL (CANCEL variable) now creates CodeElement annotation node, matching dynamic CALL behavior. RE_CANCEL_DYNAMIC with negative lookbehind. cancels[] gains isQuoted field. - #Q: RETURNING added to USING_KEYWORDS (already in prev commit) - #R: PERFORM THROUGH already fixed (THRU|THROUGH alternation) New unit tests: - Nested programs carry per-program procedureUsing - SECTION with segment number detected - Dynamic CANCEL via data item captured with isQuoted=false * feat(cobol): link PROCEDURE DIVISION USING to LINKAGE data items + close 4 findings Finding abhigyanpatwari#10 FIXED: procedureUsing parameters now create ACCESSES edges with reason 'cobol-procedure-using' from Module to matching LINKAGE SECTION Property nodes. This exposes the program's parameter contract in the graph (e.g., AUDITLOG → LS-CUST-ID, AUDITLOG → LS-AMOUNT). Findings closed by expert agent consensus: - #6 COPY IN library: WONTFIX — captured metadata, no universal library-to-directory mapping exists. Field costs nothing and is useful for library queries. - abhigyanpatwari#14 SQL DELETE: WONTFIX — DB2 requires FROM; existing FROM pattern handles it. Bare DELETE would risk false positives. - #E OCCURS DEPENDING ON: WONTFIX — runtime sizing concern, not structural. The static occurs count is sufficient for indexing. All 39 findings from 7 Claude reviews now resolved or closed. * fix(cobol): resolve 48 review findings across 9 review cycles Ninth deep review resolved all remaining COBOL parser gaps identified by 5 specialist agents (COBOL expert, architecture strategist, TypeScript reviewer, security sentinel, code simplicity reviewer). Fixes (P1 — critical): - SELECT OPTIONAL now correctly skips OPTIONAL keyword (C1) - RETURNING params excluded from PROCEDURE DIVISION USING list (C7) - SORT GIVING no longer captures clause keywords as file names (C5) - Extract flushSort() helper eliminating 40-line duplication (S2) - Flush unclosed EXEC blocks at EOF matching SORT/SELECT pattern (S3) - Guard undefined map key in jcl-processor moduleNames (S1) - Add MAX_TOTAL_EXPANSIONS=500 to prevent exponential COPY breadth (S4) Fixes (P2 — important): - Quote-aware stripInlineComment for | and *> in string literals (C2+C3) - Fixed-format literal continuation now handles quoted strings (C6) - PROGRAM-ID detected regardless of division state for siblings (C9) Fixes (P3 — cleanup): - EXEC SQL INTO restricted to INSERT INTO to avoid FETCH false-pos (C8) - Copy expander line numbers fixed from 0-based to 1-based (C11) - Remove dead code: inInStreamProc, fileIsLiteral, expansionDepth (S7-S10) Also fixes 8th-review findings: nested program CONTAINS attribution, multi-PERFORM on same line, INPUT/OUTPUT PROCEDURE IS in SORT, GO TO DEPENDING ON multi-target, MOVE CORR abbreviation, per-program procedureUsing ACCESSES edges. Tests: 145 COBOL tests passing (59 integration + 86 unit) Benchmarks: CardDemo 12,323 nodes/8,893 edges (7.4s) ACAS 14,016 nodes/15,452 edges (9.3s, -9% faster) * docs(cobol): update documentation for ninth review cycle fixes Update all 4 COBOL documentation files to reflect the 16 fixes from the ninth review cycle: - regex-extraction.md: quote-aware comment stripping, SELECT OPTIONAL, RETURNING exclusion, SORT_CLAUSE_NOISE filter, flushSort() helper, GO TO multi-target, PROGRAM-ID division-independent detection - copy-expansion.md: MAX_TOTAL_EXPANSIONS=500 breadth guard, 1-based line numbers, removed expansionDepth/warnedCircular param - deep-indexing.md: GO TO DEPENDING ON, INPUT/OUTPUT PROCEDURE IS, MOVE CORR edge reasons, INSERT INTO restriction, literal continuation - performance.md: updated benchmarks (CardDemo 12,323n/8,893e/7.4s, ACAS 14,016n/15,452e/9.3s), COPY breadth guard * fix(cobol): resolve 10th review findings — nested program edge attribution Fix 6 findings from the 10th review (PR abhigyanpatwari#498 comment #4132201110): #A+#F: All CALL/CANCEL/CICS/ENTRY/SQL/SEARCH/file-declaration edges now use owningModuleId() for nested program attribution instead of the outer program's parentId. Added helper function owningModuleId() to centralize the pattern. #B: Added USING and GIVING to SORT_CLAUSE_NOISE set to prevent MERGE USING + OUTPUT PROCEDURE from capturing clause keywords as file names. #C: INPUT/OUTPUT PROCEDURE regex now captures optional THRU/THROUGH range end paragraph, mirroring RE_PERFORM's THRU support. #D: scopedCallerLookup fallback now uses programModuleIds.get(pgm) instead of parentId, so PERFORM/MOVE/GOTO in nested programs with unresolvable paragraphs fall back to the correct inner module. #E: pendingProcUsing only set when PROCEDURE DIVISION line is NOT period-terminated, preventing false USING expectation. Tests: 145 passing | TypeScript clean * fix(cobol): resolve 10th review findings — nested program edge attribution Fix 6 findings from the 10th review (PR abhigyanpatwari#498 comment #4132201110): #A+#F: All CALL/CANCEL/CICS/ENTRY/SQL/SEARCH/file-declaration edges now use owningModuleId() for nested program attribution instead of the outer program's parentId. Added helper function owningModuleId() to centralize the pattern. #B: Added USING and GIVING to SORT_CLAUSE_NOISE set to prevent MERGE USING + OUTPUT PROCEDURE from capturing clause keywords as file names. #C: INPUT/OUTPUT PROCEDURE regex now captures optional THRU/THROUGH range end paragraph, mirroring RE_PERFORM's THRU support. #D: scopedCallerLookup fallback now uses programModuleIds.get(pgm) instead of parentId, so PERFORM/MOVE/GOTO in nested programs with unresolvable paragraphs fall back to the correct inner module. #E: pendingProcUsing only set when PROCEDURE DIVISION line is NOT period-terminated, preventing false USING expectation. Tests: 145 passing | TypeScript clean * fix(cobol): resolve 11th review findings — final nested program + multi-CALL gaps #1: scopedCallerLookup(null) now uses owningModuleId(lineNum) instead of parentId, fixing PERFORM/MOVE/GOTO before first paragraph in nested programs. #2+#3: CALL and CANCEL extraction now uses matchAll (global flag) to capture multiple occurrences on the same line. Dynamic CALL/CANCEL checked independently instead of in else branch. #4: SORT/MERGE ACCESSES edge IDs now use owningModuleId(sort.line) instead of parentId for nested program correctness. #5: preprocessCobolSource free-format detection now uses first 10 lines (consistent with extractCobolSymbolsWithRegex threshold). #6: EXCLUDED_PARA_NAMES expanded with DISPLAY, ACCEPT, WRITE, READ, REWRITE, DELETE, OPEN, CLOSE, RETURN, RELEASE, SORT, MERGE to prevent false-positive paragraph detection on isolated verbs. Also removed unused GraphNode import from cobol-processor.ts. Tests: 145 passing | TypeScript clean * docs(cobol): deepened full language coverage plan with research findings 3 research agents analyzed Phase 1-2 features and graph value ranking. Key findings: cobol-call-using is #1 edge type (9.2/10); multi-line accumulation is dominant challenge; DECLARATIVES is lowest-risk Phase 2 item; SET TO TRUE covers 80-90% of SET usage. * feat(cobol): implement Phase 1 — high-value data flow edges 4 new extraction features that create new ACCESSES and IMPORTS edges: 1.1: EXEC SQL INCLUDE -> IMPORTS edges with reason 'sql-include' Handles unquoted (SQLCA), quoted ('DBRMLIB.MEMBER'), and underscored (CUST_TBL_DCL) member names. 1.2: CALL USING parameter extraction -> ACCESSES edges Extracts parameters from CALL USING clause, filtering BY/REFERENCE/ CONTENT/VALUE/ADDRESS/OF/LENGTH/OMITTED keywords. Creates 'cobol-call-using' ACCESSES edges (graph value: 9.2/10). 1.4: OCCURS DEPENDING ON -> ACCESSES edges with reason 'cobol-depends-on' Extended OCCURS regex captures DEPENDING ON field with subscript stripping. Creates dependency edge from table to controlling field. 1.5: VALUE clause for standard data items Extracts VALUE from data item clauses: quoted strings with type prefix (X/N/G/B), ALL literals, numerics (incl negative/decimal), and figurative constants. Populates Property node values. Tests: 145 passing (+2 ACCESSES from CALL USING) | TypeScript clean * feat(cobol): implement Phase 2 — DECLARATIVES, SET, INSPECT, EXEC DLI 4 new extraction features for error handling, data flow, and IMS/DB: 2.1: EXEC DLI (IMS/DB) -> CodeElement + ACCESSES edges Accumulates EXEC DLI blocks like EXEC SQL. Parses DLI verbs (GU, GN, ISRT, REPL, DLET, CHKP, SCHD, TERM). Extracts SEGMENT, PCB, INTO/FROM, PSB. Creates dli-{verb} ACCESSES edges to <ims>:segment Record nodes. 2.2: DECLARATIVES / USE AFTER EXCEPTION -> ACCESSES edges Tracks inDeclaratives state. Detects USE AFTER STANDARD EXCEPTION ON file-name. Creates cobol-error-handler ACCESSES edge from handler section to file Record. 2.3: SET statement -> ACCESSES edges Detects SET TO TRUE (80-90% of SET usage) and SET index TO/UP BY/DOWN BY. Creates cobol-set-condition / cobol-set-index write edges + cobol-set-read for identifier values. 2.4: INSPECT -> ACCESSES edges with multi-line accumulator Accumulates INSPECT until period (like SORT). Extracts inspected field + tally counters. Creates cobol-inspect-read/write/tally edges. Form detection: tallying/replacing/converting/combined. Preprocessor: 1398 -> 1597 LOC (+199). Tests: 145 passing. * feat(cobol): implement Phase 3 — completeness fixes 6 partial features fixed to first-class support: 3.1: CALL RETURNING -> ACCESSES write edge (cobol-call-returning) 3.2: SELECT OPTIONAL flag preserved in FileDeclaration + Record node 3.3: ALTERNATE RECORD KEY extraction (matchAll for multiple keys) 3.4: COMMON attribute on nested programs (RE_PROGRAM_ID extended) 3.5: IS EXTERNAL / IS GLOBAL as first-class boolean properties (removed usage string hack) 3.6: AUTHOR / DATE-WRITTEN mapped to Module node description Tests: 145 passing | TypeScript clean * feat(cobol): implement Phase 4 — INITIALIZE + metadata completeness 4.1: INITIALIZE statement -> ACCESSES write edge (cobol-initialize) 4.2: DATE-COMPILED and INSTALLATION paragraphs extracted and mapped to Module node description alongside existing AUTHOR/DATE-WRITTEN All 4 plan phases complete. Coverage: ~95% (up from 71.9%). Tests: 145 passing | TypeScript clean * test(cobol): add 24 unit tests for Phase 1-4 features Coverage for all new extraction features: Phase 1 (8 tests): - EXEC SQL INCLUDE (unquoted, quoted, underscored) - CALL USING (simple, mixed modes, ADDRESS OF, OMITTED) - CALL RETURNING - OCCURS DEPENDING ON - VALUE clause (string, numeric, figurative constant) Phase 2 (10 tests): - EXEC DLI GU/ISRT/SCHD (verb, segment, PCB, INTO, FROM, PSB) - DECLARATIVES USE AFTER EXCEPTION (single + multiple sections) - SET TO TRUE, SET index UP BY - INSPECT TALLYING, INSPECT REPLACING Phase 3-4 (6 tests): - SELECT OPTIONAL flag - ALTERNATE RECORD KEY - PROGRAM-ID IS COMMON - IS EXTERNAL / IS GLOBAL booleans - INITIALIZE extraction - Full programMetadata (AUTHOR, DATE-WRITTEN, DATE-COMPILED, INSTALLATION) Total: 168 tests passing (145 + 24 - 1 removed duplicate) * fix(cobol): use /\r?\n/ split for Windows CRLF compatibility All 4 COBOL source files now split on /\r?\n/ instead of '\n' to handle CRLF line endings on Windows. Previously, trailing \r in lines caused RE_GOTO's $ anchor to fail on multi-line GO TO DEPENDING ON statements, producing only 1 goto edge instead of 4. Files fixed: cobol-preprocessor.ts (2 sites), cobol-processor.ts, jcl-parser.ts, cobol-copy-expander.ts Tests: 168 passing | TypeScript clean * fix(cobol): resolve 12th review — dynamic CALL/CANCEL dedup + trailing anchors #1+#2: Removed incorrect hasQuotedCall/hasQuotedCancel deduplication guards. RE_CALL_DYNAMIC and RE_CANCEL_DYNAMIC require [A-Z] after CALL/CANCEL, so they CANNOT match quoted targets — the guards were both unnecessary and actively harmful, suppressing dynamic CALL/CANCEL in ON EXCEPTION patterns. #3+#5: Changed RE_CALL_DYNAMIC and RE_CANCEL_DYNAMIC trailing anchor from (?:\s|\.) to (?=\s|\.|$) (lookahead). The consuming anchor failed when the identifier was the last token on a physical line. Tests: 168 passing | TypeScript clean * feat(cobol): add CALL accumulator + fix SORT double-statement (#4, #6) Finding #4: Multi-line CALL USING accumulator Added callAccum state variable that accumulates CALL statements spanning multiple physical lines until period or END-CALL is found. Uses flushCallAccum() to re-extract CALL target + USING parameters from the full accumulated statement. This fixes the silent loss of ACCESSES parameter edges when USING appears on lines after CALL. Finding #6: SORT double-statement on same line After flushSort(), the code now falls through to re-check the current line for a new SORT/MERGE start (was previously blocked by the sortAccum === null check evaluating before flushSort ran). Also fixed: used non-global regex for CALL detection test to avoid the classic global regex .test() lastIndex bug. Tests: 168 passing (+1 ACCESSES from multi-line CALL USING) * fix(cobol): resolve 13th review — CICS LOAD, USING extraction, file scoping #1: CICS LOAD unresolved edge no longer silently deleted in second pass. Changed narrow cics-link/cics-xctl check to catch-all pattern: rel.reason?.startsWith('cics-') && rel.reason.endsWith('-unresolved') #2: flushCallAccum USING extraction now stops before COBOL statement verbs (INSPECT, SEARCH, SORT, MERGE, DISPLAY, ACCEPT, MOVE, PERFORM, GO TO, CALL, IF, EVALUATE). Prevents absorbing adjacent statements as false USING parameters in legacy pre-COBOL-85 code without END-CALL. #3: CICS FILE Record nodes now globally-scoped (<cics-file>:FILENAME) instead of per-file-scoped. Enables cross-program CICS file access analysis, consistent with SQL table scoping (<db>:TABLE). #4: callAccum pre-check regex now has (?<![A-Z0-9-]) lookbehind to prevent false activation on compound identifiers like WS-CALL-FLAG. Tests: 168 passing | TypeScript clean * fix(cobol): resolve 14th review — callAccum false paragraph + Area A guard #1: callAccum continuation lines now check for COBOL statement verb starts (GO TO, PERFORM, MOVE, etc.) and paragraph/section headers. If detected, the CALL is flushed as-is and the line processed normally — prevents false paragraph detection and currentParagraph corruption from lines like "WS-ADDR." being treated as paragraphs. #4: callAccum pre-check now guarded by currentDivision === 'procedure' to prevent unnecessary activations in DATA DIVISION. #5: Fixed-format paragraph detection now rejects lines with >7 leading spaces (Area B indentation) as paragraph candidates. Paragraph names in fixed-format must start in Area A (col 8-11, max 7 spaces). Free-format mode is unaffected. Tests: 168 passing | TypeScript clean * fix(cobol): resolve 15th review — callAccum Area A + verb boundary fixes #A: Column-position-aware paragraph detection in callAccum flush. #B: inspectAccum early-flush on paragraph/section/verb headers. #C: Verb boundary \b → (?:\s|$) prevents MOVE-COUNT false flush. * test(cobol): add 17 edge-case regression tests + fix USING verb boundary 17 new tests covering all recurring review patterns: Multi-line CALL USING (7 tests): - Parameters on separate continuation lines (IBM mainframe style) - No absorption of INSPECT/GO TO/paragraphs following CALL - END-CALL scope terminator - Hyphenated identifiers (MOVE-COUNT) not triggering false flush - Dual quoted+dynamic CALL on same line (ON EXCEPTION) Nested program attribution (2 tests): - CALL in inner program within inner line range - PERFORM before first paragraph has null caller CRLF compatibility (1 test): - GO TO DEPENDING ON with \r\n line endings Area A paragraph detection (2 tests): - Area B (>7 spaces) rejected; Area A (7 spaces) accepted SORT/MERGE (1 test): COLLATING SEQUENCE keywords not captured PROCEDURE USING (2 tests): RETURNING excluded, period-terminated Comment stripping (1 test): pipe in quoted string preserved SELECT OPTIONAL (1 test): correct file name, not OPTIONAL keyword Bug fix: USING extraction regex verb terminators changed from \bVERB\b to \bVERB(?=\s|$) in flushCallAccum — prevents truncation on hyphenated identifiers like MOVE-COUNT, PERFORM-LIMIT. Total: 185 tests passing * test(cobol): add 32 comprehensive edge-case regression tests 13 new describe blocks covering all extraction features: - EXEC DLI: no-SEGMENT, multi-line accumulation (2 tests) - SET: multiple targets, DOWN BY, TO numeric (3 tests) - INSPECT: CONVERTING, multiple counters, tallying-replacing, paragraph flush during accumulation (4 tests) - DECLARATIVES: no-STANDARD keyword, I-O mode, post-END paragraphs (3) - COPY REPLACING: pseudotext deletion ==OLD== BY ==== (1 test) - VALUE: hex literal, negative numeric, ALL literal (3 tests) - OCCURS: TO range, fixed-size without DEPENDING ON (2 tests) - Dynamic CALL/CANCEL: end-of-line, multiple CANCELs (3 tests) - EXEC SQL: INCLUDE skips tables, SELECT INTO host vars, host variable extraction (3 tests) - INITIALIZE: target and caller context (1 test) - Nested programs: sibling scoping, PROGRAM-ID without ID DIV (2) - EXEC EOF flush: unclosed EXEC SQL flushed (1 test) - Multi-PERFORM: IF/ELSE dual PERFORM on single line (1 test) - IS EXTERNAL: USAGE not polluted by external flag (1 test) Total: 215 tests passing * fix(cobol): resolve 16th review — CANCEL in CALL block + USING boundary #1: flushCallAccum now extracts CANCEL statements from within CALL ON EXCEPTION blocks. Adds RE_CANCEL + RE_CANCEL_DYNAMIC matchAll passes alongside existing CALL extraction. #2: Added \bCANCEL(?=\s|$) to USING lookahead regex to prevent CANCEL keyword being captured as false USING parameter. #3: Multi-line CALL start now returns immediately to prevent the CALL start line from simultaneously feeding sortAccum/inspectAccum. #6: Division transitions now flush all active accumulators (callAccum, sortAccum, inspectAccum) to prevent state leakage across programs. Also added CANCEL to callAccum flush trigger verb list. Tests: 215 passing | TypeScript clean * refactor(cobol): extract shared verb constants + resolve 17th review Extract COBOL_STATEMENT_VERBS, RE_STATEMENT_VERB_START, and RE_USING_PARAMS as shared constants — eliminates 4 duplicated 25-verb regex patterns. 17th review: #1 flushCallAccum before EXEC entry, #2 inspectAccum verb parity via shared constant. Tests: 215 passing | TypeScript clean * test(cobol): replace all fuzzy assertions with exact toBe checks Replaced 7 toBeGreaterThan/toBeLessThan/toBeGreaterThanOrEqual assertions with exact toBe values: - dataItems.length: >= 3 → toBe(3) - calls.length: >= 1 → toBe(1) - calls[0].line: range check → toBe(10) - programs[].startLine/endLine: comparison → exact values - innerA.endLine/innerB.startLine: comparison → exact values Also added 11 new edge-case tests (accumulator flush on EXEC/division transitions, free-format, CANCEL in CALL block, SORT THRU, verb flush, integration). 226 tests passing — zero fuzzy assertions remain. * fix(cobol): resolve 19th review + 15 accumulator flush tests Fixes: #1: END PROGRAM flushes callAccum/sortAccum/inspectAccum #2: PROGRAM-ID sibling path flushes all accumulators #3: Added COMPUTE/ADD/SUBTRACT/MULTIPLY/DIVIDE/STRING/UNSTRING to COBOL_STATEMENT_VERBS (now 32 verbs) Tests (15 new): - END PROGRAM flush: single + nested programs (2) - PROGRAM-ID sibling flush (1) - Arithmetic verb flush: COMPUTE/ADD/SUBTRACT/MULTIPLY/DIVIDE (5) - String verb flush: STRING/UNSTRING (2) - Arithmetic not captured as false USING params (1) - SORT flushed at END PROGRAM (1) - INSPECT flushed at END PROGRAM (1) - All with exact toBe assertions (2) Total: 239 tests passing | Zero fuzzy assertions * fix(cobol): resolve 20th review — INITIALIZE multi-target + 2 tests Finding 1: INITIALIZE now captures multiple targets with REPLACING clause keyword filtering. Regex changed to lazy match stopping at REPLACING/WITH/period boundary. Targets split on whitespace and filtered against INITIALIZE_CLAUSE_KEYWORDS set. Tests (2 new): - INITIALIZE multi-target: WS-CUSTOMER WS-ORDER WS-LINE-ITEM → 3 - INITIALIZE with REPLACING: only WS-RECORD captured, not keywords Total: 241 tests passing | TypeScript clean
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.
Description
Summary
analyze(was a side effect of indexing)setupthe single owner of skill installation (global only)skills/directory instead of hardcoding themProblem
When users run both
gitnexus setupandgitnexus analyze, the same skills get installed to two locations:~/.claude/skills/gitnexus-exploring/SKILL.mdsetup(global)<repo>/.claude/skills/gitnexus/gitnexus-exploring/SKILL.mdanalyze(project-local)This triggers Claude Code bug #25209 — both copies appear in the skill list instead of one shadowing the other. Users see every GitNexus skill listed twice.
Beyond the duplication bug, skills are static markdown files that don't depend on the repository index. Installing them on every
analyzerun is unnecessary work in the wrong place. There were also two separate implementations of the same install logic.Solution
Single owner:
setupinstalls skills globally to~/.claude/skills/,~/.cursor/skills/, and~/.config/opencode/skill/.analyzeno longer touches skills.Migration:
analyzeprints a deprecation notice if stale project-local skills exist.setupcleans them up after installing globally.Auto-discovery: Skill names are discovered from the
skills/source directory at install time instead of being hardcoded. This automatically picks upgitnexus-pr-review(previously missing) and prevents future breakage when skills are added or renamed.Changes
gitnexus/src/cli/ai-context.tsinstallSkills()function and its call; cleaned up unused importsgitnexus/src/cli/analyze.tscheckStaleProjectSkills()deprecation notice; updated setup tip to mention skillsgitnexus/src/cli/setup.tsdiscoverSkillNames()replacing hardcoded list; addedcleanupProjectLocalSkills()with worktree/submodule supportgitnexus/test/unit/ai-context.test.tsanalyzeno longer installs skills; regression guards for dynamic context generationgitnexus/test/unit/setup-skills.test.tsinstallSkillsTotests,setupCommandcleanup tests,discoverSkillNamesdiscovery testsgitnexus/test/unit/analyze-skills-notice.test.tscheckStaleProjectSkillsexport and behaviorREADME.mdDesign decisions
Why not keep both locations? Adds complexity to work around a bug that shouldn't exist in our code. Skills are static config — one canonical location is cleaner.
Why doesn't
analyzedelete stale skills? After the refactor,analyzeno longer owns skills. Deleting them would cross the responsibility boundary. It warns;setupcleans up.Why auto-discover instead of hardcode? The hardcoded
SKILL_NAMESlist was the root cause ofgitnexus-pr-reviewbeing silently excluded. Discovery from disk means adding a new skill is just dropping a file — no code change required.Worktree/submodule support:
cleanupProjectLocalSkillswalks upward to find the repo root, handling.gitas either a directory (standard) or a file (worktrees/submodules).Test plan
analyzeno longer creates.claude/skills/gitnexus/(acceptance tests Update YAML formatting, rename GitNexus index, and fix CLI args #1-2)analyzeprints deprecation notice when stale skills exist (The packages folder is being ignored, but it’s actually part of the core project. abhigyanpatwari/GitNexus#15-17)analyzeshows notice even on "Already up to date" early returnsetupinstalls all 7 discovered skills with non-empty SKILL.md (highlight tool fixes abhigyanpatwari/GitNexus#7-9)setupremoves project-local skills in git repos (fixed LLM provider config warning issue abhigyanpatwari/GitNexus#11-12)setuphandles nested dirs, worktrees, non-git dirs, empty dirs (reveted breaking changes abhigyanpatwari/GitNexus#13-14, nested/worktree tests)discoverSkillNamesdiscovers flat files, directories, mixed layouts (Add database indexes for query performance optimization abhigyanpatwari/GitNexus#27-33)discoverSkillNamesfilters togitnexus-*prefix only (Mcp bridge abhigyanpatwari/GitNexus#28)discoverSkillNamesignores directories without SKILL.md (Leidens algorithm and processmap abhigyanpatwari/GitNexus#33)Summary by CodeRabbit
New Features
Refactor
Documentation
Tests