From 2f268fc87fb55677c92f8a07ea2c2eb5fdd95a83 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gerg=C5=91=20Magyar?= Date: Sat, 10 Oct 2026 19:31:02 +0300 Subject: [PATCH] fix(rename): prevent unrelated edits during symbol renames --- .claude/skills/gitnexus-refactoring/SKILL.md | 35 +- .github/workflows/ci-quality.yml | 4 +- .husky/pre-commit | 2 +- AGENTS.md | 2 +- CLAUDE.md | 2 +- CONTRIBUTING.md | 2 +- GUARDRAILS.md | 2 +- README.md | 47 ++- .../skills/gitnexus-refactoring/SKILL.md | 35 +- .../skills/gitnexus-refactoring/SKILL.md | 35 +- gitnexus/package-lock.json | 15 + gitnexus/package.json | 3 +- gitnexus/skills/gitnexus-refactoring.md | 35 +- gitnexus/src/cli/ai-context.ts | 2 +- gitnexus/src/mcp/local/local-backend.ts | 212 +--------- gitnexus/src/mcp/local/rename/rename-plan.ts | 313 +++++++++++++++ .../src/mcp/local/rename/typescript-rename.ts | 359 +++++++++++++++++ gitnexus/src/mcp/read-only-policy.ts | 1 + gitnexus/src/mcp/resources.ts | 8 +- gitnexus/src/mcp/tools.ts | 62 +-- gitnexus/test/unit/calltool-dispatch.test.ts | 40 +- gitnexus/test/unit/mcp-read-only.test.ts | 32 +- gitnexus/test/unit/rename-edit-report.test.ts | 327 +++++++--------- gitnexus/test/unit/rename-plan.test.ts | 185 +++++++++ .../test/unit/shipped-skills-sync.test.ts | 7 +- gitnexus/test/unit/tools.test.ts | 16 +- gitnexus/test/unit/typescript-rename.test.ts | 369 ++++++++++++++++++ 27 files changed, 1654 insertions(+), 498 deletions(-) create mode 100644 gitnexus/src/mcp/local/rename/rename-plan.ts create mode 100644 gitnexus/src/mcp/local/rename/typescript-rename.ts create mode 100644 gitnexus/test/unit/rename-plan.test.ts create mode 100644 gitnexus/test/unit/typescript-rename.test.ts diff --git a/.claude/skills/gitnexus-refactoring/SKILL.md b/.claude/skills/gitnexus-refactoring/SKILL.md index 7bcc12711..c8ca26147 100644 --- a/.claude/skills/gitnexus-refactoring/SKILL.md +++ b/.claude/skills/gitnexus-refactoring/SKILL.md @@ -54,9 +54,9 @@ checkout and reports nothing changed, which reads as a verified refactor. ``` - [ ] list_repos {} — bind repo; explicit repo when >1 indexed, ask if ambiguous -- [ ] rename({symbol_name: "oldName", new_name: "newName", dry_run: true}) — preview all edits +- [ ] rename_preview({symbol_name: "oldName", new_name: "newName"}) — preview semantic edits - [ ] Confirm the previewed file paths are in the bound repository/worktree -- [ ] Review graph edits (high confidence) and text_search edits (review carefully) +- [ ] Review exact semantic spans and coverage.tsconfig_path; select tsconfig_path if ambiguous - [ ] If satisfied: rename({..., dry_run: false}) — apply edits - [ ] detect_changes() — verify only expected files changed - [ ] Run tests for affected processes @@ -89,15 +89,30 @@ checkout and reports nothing changed, which reads as a verified refactor. ## Tools -**rename** — automated multi-file rename: +**rename_preview** — read-only semantic rename preview: ``` -rename({symbol_name: "validateUser", new_name: "authenticateUser", repo: "my-app", dry_run: true}) +rename_preview({symbol_name: "validateUser", new_name: "authenticateUser", repo: "my-app"}) → 12 edits across 8 files -→ 10 graph edits (high confidence), 2 text_search edits (review) -→ Changes: [{file_path, edits: [{line, old_text, new_text, confidence}]}] +→ semantic_edits: 12, coverage: {scope: "project", tsconfig_path: "tsconfig.json"} +→ changes: [{file_path, edits: [{start, length, line, old_text, new_text, confidence: "semantic"}]}] ``` +Only configured TypeScript/JavaScript projects are supported. If multiple configs +include the declaration, pass a returned `tsconfig_path` candidate. Coverage is +limited to that project; project references and unsupported languages fail closed. +There is no regex fallback: `graph_edits` and `text_search_edits` are zero. +Comments, strings, and unrelated same-spelled symbols are not replacements. + +`rename` remains available with `dry_run: true` (default) or explicit +`dry_run: false`. Both use the same planner. Results use `result_version: 2`; +`total_edits` counts occurrences, not matching lines. Import/export alias affixes +are part of `new_text`. Separate preview and apply calls recompute the plan. +Check `planning_status`, `application_status`, and `applied`. After a write +failure, `changes` and counts describe only landed edits; inspect `failed_files` +before retrying. New-name collisions still require compiler and test verification. +Neither tool accepts `branch`: they inspect the current checkout. + **impact** — map all dependents first: ``` @@ -146,11 +161,11 @@ RETURN caller.name, caller.filePath ORDER BY caller.filePath 0. list_repos {} → total: 2 (my-app, billing-api) — both define validateUser, so bind explicitly -1. rename({symbol_name: "validateUser", new_name: "authenticateUser", repo: "my-app", dry_run: true}) - → 12 edits: 10 graph (safe), 2 text_search (review) - → Files: validator.ts, login.ts, middleware.ts, config.json... +1. rename_preview({symbol_name: "validateUser", new_name: "authenticateUser", repo: "my-app"}) + → 12 semantic occurrences in the selected TypeScript project + → Files: validator.ts, login.ts, middleware.ts... -2. Review text_search edits (config.json: dynamic reference!) +2. Review semantic spans and project coverage; inspect dynamic references separately 3. rename({symbol_name: "validateUser", new_name: "authenticateUser", repo: "my-app", dry_run: false}) → Applied 12 edits across 8 files diff --git a/.github/workflows/ci-quality.yml b/.github/workflows/ci-quality.yml index dedf5edf6..bd51aa880 100644 --- a/.github/workflows/ci-quality.yml +++ b/.github/workflows/ci-quality.yml @@ -51,7 +51,9 @@ jobs: - uses: ./.github/actions/setup-gitnexus with: lifecycle-scripts: 'false' - - run: npx tsc --noEmit + # The runtime language-service alias also publishes a tsc bin. Select the + # build compiler explicitly so npm's bin-link order cannot change CI. + - run: node node_modules/typescript/lib/tsc.js --noEmit working-directory: gitnexus - name: Typecheck tests run: npm run typecheck:tests diff --git a/.husky/pre-commit b/.husky/pre-commit index 2a04a77f0..180a184b4 100755 --- a/.husky/pre-commit +++ b/.husky/pre-commit @@ -20,7 +20,7 @@ fi if [ -n "$CLI_CHANGED" ]; then echo "pre-commit: typechecking gitnexus..." - cd "$ROOT/gitnexus" && ./node_modules/.bin/tsc --noEmit || exit 1 + cd "$ROOT/gitnexus" && node node_modules/typescript/lib/tsc.js --noEmit || exit 1 fi echo "pre-commit: all checks passed" diff --git a/AGENTS.md b/AGENTS.md index 2172825f5..8dd91d20e 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -134,7 +134,7 @@ This project is indexed by GitNexus as **GitNexus** (248612 symbols, 565510 rela - NEVER edit a function, class, or method before MCP/CLI impact analysis. - NEVER ignore HIGH or CRITICAL risk warnings from impact analysis, and never read `UNKNOWN` as an all-clear — it means the walk could not answer, which is the one verdict that requires confirming by other means. -- NEVER rename symbols with find-and-replace — use `rename` which understands the call graph. +- NEVER rename symbols with find-and-replace — use `rename` for configured TS/JS projects after reviewing `rename_preview` semantic spans and project coverage. - NEVER commit before MCP/CLI graph change analysis. ## Resources diff --git a/CLAUDE.md b/CLAUDE.md index 0d418c9b6..f37848080 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -81,7 +81,7 @@ This project is indexed by GitNexus as **GitNexus** (248612 symbols, 565510 rela - NEVER edit a function, class, or method before MCP/CLI impact analysis. - NEVER ignore HIGH or CRITICAL risk warnings from impact analysis, and never read `UNKNOWN` as an all-clear — it means the walk could not answer, which is the one verdict that requires confirming by other means. -- NEVER rename symbols with find-and-replace — use `rename` which understands the call graph. +- NEVER rename symbols with find-and-replace — use `rename` for configured TS/JS projects after reviewing `rename_preview` semantic spans and project coverage. - NEVER commit before MCP/CLI graph change analysis. ## Resources diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index dc9cb884d..b2fa39487 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -75,7 +75,7 @@ Commits within a PR may use any style — only the **merged PR title** shows up ## Before you open a PR - [ ] Tests pass for the packages you touched (`gitnexus` and/or `gitnexus-web`). -- [ ] Typecheck passes: `npx tsc --noEmit` in `gitnexus/` and `npx tsc -b --noEmit` in `gitnexus-web/`. Those commands use TypeScript 7. The web app tsconfig lists `lib` `DOM`/`DOM.Iterable` and `jsx: react-jsx` so React/JSX typecheck on 7; Vite/`vitest` keep `@vitejs/plugin-react` with the automatic JSX runtime. Build `gitnexus-shared/dist` first with a parent `lib/tsc.js` (a `gitnexus` install/`npm run build` does this). Repo ESLint stays syntax-only on a TypeScript 5.x peer until typescript-eslint supports 7. +- [ ] Typecheck passes: `node node_modules/typescript/lib/tsc.js --noEmit` in `gitnexus/` and `npx tsc -b --noEmit` in `gitnexus-web/`. Those commands use TypeScript 7; the explicit CLI package path avoids npm bin-link collisions with the pinned rename language-service alias. The web app tsconfig lists `lib` `DOM`/`DOM.Iterable` and `jsx: react-jsx` so React/JSX typecheck on 7; Vite/`vitest` keep `@vitejs/plugin-react` with the automatic JSX runtime. Build `gitnexus-shared/dist` first with a parent `lib/tsc.js` (a `gitnexus` install/`npm run build` does this). Repo ESLint stays syntax-only on a TypeScript 5.x peer until typescript-eslint supports 7. - [ ] No secrets, tokens, or machine-specific paths committed. - [ ] Documentation updated if behavior or public CLI/MCP contract changes. - [ ] Every new `GITNEXUS_*` environment variable has a row in the **Environment variables** table in [README.md](README.md) — variable, default, effect, and when to tune it. diff --git a/GUARDRAILS.md b/GUARDRAILS.md index 354be789c..a3ccd9fc8 100644 --- a/GUARDRAILS.md +++ b/GUARDRAILS.md @@ -16,7 +16,7 @@ Maintainer may widen scope per task. ## Non-negotiables 1. **Never commit secrets** — API keys, tokens, real `.env` values, private URLs, session cookies. Use `.env.example` with placeholders. -2. **Never rename with find-and-replace** in GitNexus-indexed projects — use `rename` MCP tool with `dry_run: true` first, review `graph` vs `text_search` edits. No separate `gitnexus rename` CLI exists. +2. **Never rename with find-and-replace** in GitNexus-indexed projects — use `rename_preview` first for supported configured TS/JS projects, review exact semantic spans and project coverage, then apply with `rename` and `dry_run: false`. Unsupported targets fail closed; inspect and refactor them manually with language-aware tools. No separate `gitnexus rename` CLI exists. 3. **Run impact analysis before editing shared symbols** — `impact` (upstream) for functions/classes/methods others call. Do not ignore HIGH/CRITICAL without maintainer sign-off. 4. **Run `detect_changes` before commit** — confirm diffs map to expected symbols/processes when the graph is available. 5. **Preserve embeddings** — plain `npx gitnexus analyze` now preserves any embeddings recorded in the index metadata (`.gitnexus/gitnexus.json`, mirrored to the legacy `meta.json`) — the previous behavior wiped them. Use `--embeddings` to also generate vectors for new/changed nodes; use `--drop-embeddings` only when an explicit wipe is intended (e.g., model swap). diff --git a/README.md b/README.md index d89a7ef05..bfcf2c3ac 100644 --- a/README.md +++ b/README.md @@ -158,7 +158,7 @@ flowchart TB ## What Your AI Agent Gets -### 19 MCP tools (17 per-repo + 2 group) +### 20 MCP tools (18 per-repo + 2 group) | Tool | What It Does | | ---------------- | ---------------------------------------------------------------------- | @@ -169,7 +169,8 @@ flowchart TB | `trace` | Shortest directed path between two symbols (call + class-member edges) | | `detect_changes` | Git-diff impact — maps changed lines to affected processes | | `check` | Read-only structural checks against the indexed graph | -| `rename` | Multi-file coordinated rename with graph + text search | +| `rename_preview` | Read-only semantic rename preview for a configured TS/JS project | +| `rename` | Preview or apply exact semantic rename occurrences | | `cypher` | Raw Cypher graph queries | | `route_map` | API route map — which components fetch which endpoints, and handlers | | `tool_map` | MCP/RPC tool definitions — where they're defined and handled | @@ -372,7 +373,7 @@ codex plugin marketplace add abhigyanpatwari/GitNexus
MCP read-only mode -Set `GITNEXUS_MCP_READ_ONLY=1` before starting the MCP server to expose only the proven single-repository read surface. Raw `cypher`, rename and group tools, group routing, and group resources are omitted from discovery and rejected before backend dispatch. Tool descriptions and generated setup/context resources are scrubbed so they do not recommend unavailable routes. +Set `GITNEXUS_MCP_READ_ONLY=1` before starting the MCP server to expose only the proven single-repository read surface. The read-only `rename_preview` is available. Raw `cypher`, destructive `rename`, and group tools, group routing, and group resources are omitted from discovery and rejected before backend dispatch. Tool descriptions and generated setup/context resources are scrubbed so they do not recommend unavailable routes. The default is unchanged when the variable is unset or `0`. Any other value fails server startup rather than silently weakening the policy. @@ -874,19 +875,49 @@ changed_symbols: [validateUser, AuthService, ...] affected_processes: [LoginFlow, RegistrationFlow, ...] ``` -### Rename (Multi-File) +### Rename (Configured TypeScript/JavaScript Project) +```js +rename_preview({symbol_name: "validateUser", new_name: "verifyUser", repo: "my-app"}) +// If project selection is ambiguous, repeat with a returned tsconfig_path. ``` -rename({symbol_name: "validateUser", new_name: "verifyUser", dry_run: true}) +```text +result_version: 2 status: success +planning_status: ready +application_status: not_requested +applied: false files_affected: 5 total_edits: 8 -graph_edits: 6 (high confidence) -text_search_edits: 2 (review carefully) -changes: [...] +semantic_edits: 8 +graph_edits: 0 +text_search_edits: 0 +text_search: not_used +coverage: {name: "typescript", version: "5.9.3", scope: "project", tsconfig_path: "tsconfig.json", ...} +changes: [{file_path, edits: [{start, length, old_text, new_text, line, confidence: "semantic"}]}] ``` +Review the exact UTF-16 spans and selected project, then call `rename` with the +same selectors and `dry_run: false` to apply. `rename` still defaults to preview. +Counts describe occurrences, including multiple edits on one line. Import/export +aliases include compiler-provided prefix/suffix text in `new_text`. Comments, +strings, and unrelated same-spelled symbols are not text-search replacements. + +Coverage is limited to one configured project, not the whole repository. Select +`tsconfig_path` explicitly when several configs contain the declaration. Unsupported +languages, project references, stale declarations, and unresolved targets fail +closed without regex fallback. Both tools operate on the checkout and reject +`branch`. The runtime compiler is pinned independently of the build compiler. + +Preview and apply recompute their plans independently. Source snapshots are +checked before writes; this is not a cross-call preview token or transactional +filesystem operation. On a write failure, `status: partial` and `failed_files` +report the failure; `applied`, counts, and `changes` describe only landed edits. +Inspect failures before retrying, and run type checking, tests, and +`detect_changes` after applying. A valid identifier is not a guarantee against +new-name collisions. + ### Cypher Queries ```cypher diff --git a/gitnexus-claude-plugin/skills/gitnexus-refactoring/SKILL.md b/gitnexus-claude-plugin/skills/gitnexus-refactoring/SKILL.md index 7bcc12711..c8ca26147 100644 --- a/gitnexus-claude-plugin/skills/gitnexus-refactoring/SKILL.md +++ b/gitnexus-claude-plugin/skills/gitnexus-refactoring/SKILL.md @@ -54,9 +54,9 @@ checkout and reports nothing changed, which reads as a verified refactor. ``` - [ ] list_repos {} — bind repo; explicit repo when >1 indexed, ask if ambiguous -- [ ] rename({symbol_name: "oldName", new_name: "newName", dry_run: true}) — preview all edits +- [ ] rename_preview({symbol_name: "oldName", new_name: "newName"}) — preview semantic edits - [ ] Confirm the previewed file paths are in the bound repository/worktree -- [ ] Review graph edits (high confidence) and text_search edits (review carefully) +- [ ] Review exact semantic spans and coverage.tsconfig_path; select tsconfig_path if ambiguous - [ ] If satisfied: rename({..., dry_run: false}) — apply edits - [ ] detect_changes() — verify only expected files changed - [ ] Run tests for affected processes @@ -89,15 +89,30 @@ checkout and reports nothing changed, which reads as a verified refactor. ## Tools -**rename** — automated multi-file rename: +**rename_preview** — read-only semantic rename preview: ``` -rename({symbol_name: "validateUser", new_name: "authenticateUser", repo: "my-app", dry_run: true}) +rename_preview({symbol_name: "validateUser", new_name: "authenticateUser", repo: "my-app"}) → 12 edits across 8 files -→ 10 graph edits (high confidence), 2 text_search edits (review) -→ Changes: [{file_path, edits: [{line, old_text, new_text, confidence}]}] +→ semantic_edits: 12, coverage: {scope: "project", tsconfig_path: "tsconfig.json"} +→ changes: [{file_path, edits: [{start, length, line, old_text, new_text, confidence: "semantic"}]}] ``` +Only configured TypeScript/JavaScript projects are supported. If multiple configs +include the declaration, pass a returned `tsconfig_path` candidate. Coverage is +limited to that project; project references and unsupported languages fail closed. +There is no regex fallback: `graph_edits` and `text_search_edits` are zero. +Comments, strings, and unrelated same-spelled symbols are not replacements. + +`rename` remains available with `dry_run: true` (default) or explicit +`dry_run: false`. Both use the same planner. Results use `result_version: 2`; +`total_edits` counts occurrences, not matching lines. Import/export alias affixes +are part of `new_text`. Separate preview and apply calls recompute the plan. +Check `planning_status`, `application_status`, and `applied`. After a write +failure, `changes` and counts describe only landed edits; inspect `failed_files` +before retrying. New-name collisions still require compiler and test verification. +Neither tool accepts `branch`: they inspect the current checkout. + **impact** — map all dependents first: ``` @@ -146,11 +161,11 @@ RETURN caller.name, caller.filePath ORDER BY caller.filePath 0. list_repos {} → total: 2 (my-app, billing-api) — both define validateUser, so bind explicitly -1. rename({symbol_name: "validateUser", new_name: "authenticateUser", repo: "my-app", dry_run: true}) - → 12 edits: 10 graph (safe), 2 text_search (review) - → Files: validator.ts, login.ts, middleware.ts, config.json... +1. rename_preview({symbol_name: "validateUser", new_name: "authenticateUser", repo: "my-app"}) + → 12 semantic occurrences in the selected TypeScript project + → Files: validator.ts, login.ts, middleware.ts... -2. Review text_search edits (config.json: dynamic reference!) +2. Review semantic spans and project coverage; inspect dynamic references separately 3. rename({symbol_name: "validateUser", new_name: "authenticateUser", repo: "my-app", dry_run: false}) → Applied 12 edits across 8 files diff --git a/gitnexus-cursor-integration/skills/gitnexus-refactoring/SKILL.md b/gitnexus-cursor-integration/skills/gitnexus-refactoring/SKILL.md index 7bcc12711..c8ca26147 100644 --- a/gitnexus-cursor-integration/skills/gitnexus-refactoring/SKILL.md +++ b/gitnexus-cursor-integration/skills/gitnexus-refactoring/SKILL.md @@ -54,9 +54,9 @@ checkout and reports nothing changed, which reads as a verified refactor. ``` - [ ] list_repos {} — bind repo; explicit repo when >1 indexed, ask if ambiguous -- [ ] rename({symbol_name: "oldName", new_name: "newName", dry_run: true}) — preview all edits +- [ ] rename_preview({symbol_name: "oldName", new_name: "newName"}) — preview semantic edits - [ ] Confirm the previewed file paths are in the bound repository/worktree -- [ ] Review graph edits (high confidence) and text_search edits (review carefully) +- [ ] Review exact semantic spans and coverage.tsconfig_path; select tsconfig_path if ambiguous - [ ] If satisfied: rename({..., dry_run: false}) — apply edits - [ ] detect_changes() — verify only expected files changed - [ ] Run tests for affected processes @@ -89,15 +89,30 @@ checkout and reports nothing changed, which reads as a verified refactor. ## Tools -**rename** — automated multi-file rename: +**rename_preview** — read-only semantic rename preview: ``` -rename({symbol_name: "validateUser", new_name: "authenticateUser", repo: "my-app", dry_run: true}) +rename_preview({symbol_name: "validateUser", new_name: "authenticateUser", repo: "my-app"}) → 12 edits across 8 files -→ 10 graph edits (high confidence), 2 text_search edits (review) -→ Changes: [{file_path, edits: [{line, old_text, new_text, confidence}]}] +→ semantic_edits: 12, coverage: {scope: "project", tsconfig_path: "tsconfig.json"} +→ changes: [{file_path, edits: [{start, length, line, old_text, new_text, confidence: "semantic"}]}] ``` +Only configured TypeScript/JavaScript projects are supported. If multiple configs +include the declaration, pass a returned `tsconfig_path` candidate. Coverage is +limited to that project; project references and unsupported languages fail closed. +There is no regex fallback: `graph_edits` and `text_search_edits` are zero. +Comments, strings, and unrelated same-spelled symbols are not replacements. + +`rename` remains available with `dry_run: true` (default) or explicit +`dry_run: false`. Both use the same planner. Results use `result_version: 2`; +`total_edits` counts occurrences, not matching lines. Import/export alias affixes +are part of `new_text`. Separate preview and apply calls recompute the plan. +Check `planning_status`, `application_status`, and `applied`. After a write +failure, `changes` and counts describe only landed edits; inspect `failed_files` +before retrying. New-name collisions still require compiler and test verification. +Neither tool accepts `branch`: they inspect the current checkout. + **impact** — map all dependents first: ``` @@ -146,11 +161,11 @@ RETURN caller.name, caller.filePath ORDER BY caller.filePath 0. list_repos {} → total: 2 (my-app, billing-api) — both define validateUser, so bind explicitly -1. rename({symbol_name: "validateUser", new_name: "authenticateUser", repo: "my-app", dry_run: true}) - → 12 edits: 10 graph (safe), 2 text_search (review) - → Files: validator.ts, login.ts, middleware.ts, config.json... +1. rename_preview({symbol_name: "validateUser", new_name: "authenticateUser", repo: "my-app"}) + → 12 semantic occurrences in the selected TypeScript project + → Files: validator.ts, login.ts, middleware.ts... -2. Review text_search edits (config.json: dynamic reference!) +2. Review semantic spans and project coverage; inspect dynamic references separately 3. rename({symbol_name: "validateUser", new_name: "authenticateUser", repo: "my-app", dry_run: false}) → Applied 12 edits across 8 files diff --git a/gitnexus/package-lock.json b/gitnexus/package-lock.json index 2afc674e5..0b067f4a8 100644 --- a/gitnexus/package-lock.json +++ b/gitnexus/package-lock.json @@ -62,6 +62,7 @@ "tree-sitter-ruby": "0.23.1", "tree-sitter-rust": "0.24.0", "tree-sitter-typescript": "0.23.2", + "typescript-language-service": "npm:typescript@5.9.3", "uuid": "^14.0.0" }, "bin": { @@ -5059,6 +5060,20 @@ "@typescript/typescript-win32-x64": "7.0.2" } }, + "node_modules/typescript-language-service": { + "name": "typescript", + "version": "5.9.3", + "resolved": "https://registry.npmjs.org/typescript/-/typescript-5.9.3.tgz", + "integrity": "sha512-jl1vZzPDinLr9eUt3J/t7V6FgNEw9QjvBPdysz9KfQDD41fQrC2Y4vKQdiaUpFT4bXlb1RHhLpp8wtm6M5TgSw==", + "license": "Apache-2.0", + "bin": { + "tsc": "bin/tsc", + "tsserver": "bin/tsserver" + }, + "engines": { + "node": ">=14.17" + } + }, "node_modules/typical": { "version": "7.3.0", "resolved": "https://registry.npmjs.org/typical/-/typical-7.3.0.tgz", diff --git a/gitnexus/package.json b/gitnexus/package.json index 8b9ccc5bc..4aaae0216 100644 --- a/gitnexus/package.json +++ b/gitnexus/package.json @@ -52,7 +52,7 @@ "serve": "tsx src/cli/index.ts serve", "dev": "tsx watch src/cli/index.ts", "test": "vitest run", - "typecheck:tests": "tsc --noEmit -p tsconfig.test.json", + "typecheck:tests": "node node_modules/typescript/lib/tsc.js --noEmit -p tsconfig.test.json", "test:unit": "vitest run test/unit", "pretest:integration": "node scripts/build.js", "test:integration": "vitest run test/integration", @@ -106,6 +106,7 @@ "tree-sitter-ruby": "0.23.1", "tree-sitter-rust": "0.24.0", "tree-sitter-typescript": "0.23.2", + "typescript-language-service": "npm:typescript@5.9.3", "uuid": "^14.0.0" }, "gitnexusEmbeddingStack": { diff --git a/gitnexus/skills/gitnexus-refactoring.md b/gitnexus/skills/gitnexus-refactoring.md index 7bcc12711..c8ca26147 100644 --- a/gitnexus/skills/gitnexus-refactoring.md +++ b/gitnexus/skills/gitnexus-refactoring.md @@ -54,9 +54,9 @@ checkout and reports nothing changed, which reads as a verified refactor. ``` - [ ] list_repos {} — bind repo; explicit repo when >1 indexed, ask if ambiguous -- [ ] rename({symbol_name: "oldName", new_name: "newName", dry_run: true}) — preview all edits +- [ ] rename_preview({symbol_name: "oldName", new_name: "newName"}) — preview semantic edits - [ ] Confirm the previewed file paths are in the bound repository/worktree -- [ ] Review graph edits (high confidence) and text_search edits (review carefully) +- [ ] Review exact semantic spans and coverage.tsconfig_path; select tsconfig_path if ambiguous - [ ] If satisfied: rename({..., dry_run: false}) — apply edits - [ ] detect_changes() — verify only expected files changed - [ ] Run tests for affected processes @@ -89,15 +89,30 @@ checkout and reports nothing changed, which reads as a verified refactor. ## Tools -**rename** — automated multi-file rename: +**rename_preview** — read-only semantic rename preview: ``` -rename({symbol_name: "validateUser", new_name: "authenticateUser", repo: "my-app", dry_run: true}) +rename_preview({symbol_name: "validateUser", new_name: "authenticateUser", repo: "my-app"}) → 12 edits across 8 files -→ 10 graph edits (high confidence), 2 text_search edits (review) -→ Changes: [{file_path, edits: [{line, old_text, new_text, confidence}]}] +→ semantic_edits: 12, coverage: {scope: "project", tsconfig_path: "tsconfig.json"} +→ changes: [{file_path, edits: [{start, length, line, old_text, new_text, confidence: "semantic"}]}] ``` +Only configured TypeScript/JavaScript projects are supported. If multiple configs +include the declaration, pass a returned `tsconfig_path` candidate. Coverage is +limited to that project; project references and unsupported languages fail closed. +There is no regex fallback: `graph_edits` and `text_search_edits` are zero. +Comments, strings, and unrelated same-spelled symbols are not replacements. + +`rename` remains available with `dry_run: true` (default) or explicit +`dry_run: false`. Both use the same planner. Results use `result_version: 2`; +`total_edits` counts occurrences, not matching lines. Import/export alias affixes +are part of `new_text`. Separate preview and apply calls recompute the plan. +Check `planning_status`, `application_status`, and `applied`. After a write +failure, `changes` and counts describe only landed edits; inspect `failed_files` +before retrying. New-name collisions still require compiler and test verification. +Neither tool accepts `branch`: they inspect the current checkout. + **impact** — map all dependents first: ``` @@ -146,11 +161,11 @@ RETURN caller.name, caller.filePath ORDER BY caller.filePath 0. list_repos {} → total: 2 (my-app, billing-api) — both define validateUser, so bind explicitly -1. rename({symbol_name: "validateUser", new_name: "authenticateUser", repo: "my-app", dry_run: true}) - → 12 edits: 10 graph (safe), 2 text_search (review) - → Files: validator.ts, login.ts, middleware.ts, config.json... +1. rename_preview({symbol_name: "validateUser", new_name: "authenticateUser", repo: "my-app"}) + → 12 semantic occurrences in the selected TypeScript project + → Files: validator.ts, login.ts, middleware.ts... -2. Review text_search edits (config.json: dynamic reference!) +2. Review semantic spans and project coverage; inspect dynamic references separately 3. rename({symbol_name: "validateUser", new_name: "authenticateUser", repo: "my-app", dry_run: false}) → Applied 12 edits across 8 files diff --git a/gitnexus/src/cli/ai-context.ts b/gitnexus/src/cli/ai-context.ts index 89bf912f0..762d9b536 100644 --- a/gitnexus/src/cli/ai-context.ts +++ b/gitnexus/src/cli/ai-context.ts @@ -250,7 +250,7 @@ This project is indexed by GitNexus as **${projectName}**${noStats ? '' : ` (${s - NEVER edit a function, class, or method before MCP/CLI impact analysis. - NEVER ignore HIGH or CRITICAL risk warnings from impact analysis, and never read \`UNKNOWN\` as an all-clear — it means the walk could not answer, which is the one verdict that requires confirming by other means. -- NEVER rename symbols with find-and-replace — use \`rename\` which understands the call graph. +- NEVER rename symbols with find-and-replace — use \`rename\` for configured TS/JS projects after reviewing \`rename_preview\` semantic spans and project coverage. - NEVER commit before MCP/CLI graph change analysis. ## Resources diff --git a/gitnexus/src/mcp/local/local-backend.ts b/gitnexus/src/mcp/local/local-backend.ts index e5dc607f1..95fdaa28e 100644 --- a/gitnexus/src/mcp/local/local-backend.ts +++ b/gitnexus/src/mcp/local/local-backend.ts @@ -151,6 +151,7 @@ import { CHECKOUT_SOURCE_TOOLS, READ_FILE_DEFAULT_MAX_LINES, } from '../tools.js'; +import { renameSymbol } from './rename/rename-plan.js'; import { foldNumericToolArgumentAliases } from '../tool-arguments.js'; import { findImportCycles, IMPORT_CYCLE_LIMIT } from '../../core/graph/import-cycles.js'; import { decodeTaintPath } from '../../core/ingestion/taint/path-codec.js'; @@ -2999,6 +3000,10 @@ export class LocalBackend { return this.check(repo, p); case 'rename': return this.rename(repo, p as unknown as Parameters[1]); + case 'rename_preview': + return this.rename(repo, { ...p, dry_run: true } as unknown as Parameters< + LocalBackend['rename'] + >[1]); // Legacy aliases for backwards compatibility case 'search': return this.query(repo, p); @@ -6744,11 +6749,7 @@ export class LocalBackend { }; } - /** - * Rename tool — multi-file coordinated rename using graph + text search. - * Graph refs are tagged "graph" (high confidence). - * Additional refs found via text search are tagged "text_search" (lower confidence). - */ + /** Resolve graph identity, then let the language provider find exact occurrences. */ private async rename( repo: RepoHandle, params: { @@ -6756,213 +6757,22 @@ export class LocalBackend { symbol_uid?: string; new_name: string; file_path?: string; + tsconfig_path?: string; dry_run?: boolean; }, ): Promise { await this.ensureInitialized(repo); - - const { new_name, file_path } = params; - const dry_run = params.dry_run ?? true; - if (!params.symbol_name && !params.symbol_uid) { return { error: 'Either symbol_name or symbol_uid is required.' }; } - - /** Guard: ensure a file path resolves within the repo root (prevents path traversal) */ - const assertSafePath = (filePath: string): string => { - const full = path.resolve(repo.repoPath, filePath); - const safePrefix = repo.repoPath.endsWith(path.sep) - ? repo.repoPath - : repo.repoPath + path.sep; - if (!full.startsWith(safePrefix) && full !== repo.repoPath) { - throw new Error(`Path traversal blocked: ${filePath}`); - } - return full; - }; - - // Step 1: Find the target symbol (reuse context's lookup) const lookupResult = await this.context(repo, { name: params.symbol_name, uid: params.symbol_uid, - file_path, + file_path: params.file_path, }); - - if (lookupResult.status === 'ambiguous') { - return lookupResult; // pass disambiguation through - } - if (lookupResult.error) { - return lookupResult; - } - - const sym = lookupResult.symbol; - const oldName = sym.name; - - if (oldName === new_name) { - return { error: 'New name is the same as the current name.' }; - } - - // Steps 2+3: Determine the set of files the apply step will rewrite, then - // enumerate every occurrence in each. The apply step (Step 4) does a - // whole-file `\boldName\b` global replace on every file in `changes`, so the - // reported edit list MUST enumerate every matching line in every such file — - // otherwise the preview under-reports what lands, and the same partial list - // comes back after apply (#2605). Building `changes` from one file set makes - // the preview enumerate exactly the files the apply loop rewrites, using the - // same word-boundary regex. (This is per-call consistency; the apply loop - // still re-reads each file, so an external write landing between preview and - // apply is a pre-existing gap this method does not lock against.) - type RenameEdit = { - line: number; - old_text: string; - new_text: string; - confidence: 'graph' | 'text_search'; - }; - const escapedOldName = oldName.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'); - - // Classify each file to rewrite by how it was discovered. Definition and - // graph-ref files carry graph confidence; files found only by text search - // carry text_search confidence. A graph-classified file is never downgraded. - const fileConfidence = new Map(); - - if (sym.filePath) { - fileConfidence.set(sym.filePath, 'graph'); - } - - // All incoming refs from graph (callers, importers, etc.) - const allIncoming = [ - ...(lookupResult.incoming.calls || []), - ...(lookupResult.incoming.imports || []), - ...(lookupResult.incoming.extends || []), - ...(lookupResult.incoming.implements || []), - ]; - for (const ref of allIncoming) { - if (ref.filePath) { - fileConfidence.set(ref.filePath, 'graph'); - } - } - - // Text search for files the graph might have missed entirely. - try { - const { execFileSync } = await import('child_process'); - const rgArgs = [ - '-l', - '--type-add', - 'code:*.{ts,tsx,js,jsx,py,go,rs,java,c,h,cpp,cc,cxx,hpp,hxx,hh,cs,php,swift}', - '-t', - 'code', - `\\b${oldName}\\b`, - '.', - ]; - const output = execFileSync('rg', rgArgs, { - cwd: repo.repoPath, - encoding: 'utf-8', - timeout: 5000, - // Avoid ENOBUFS on large repos: rg -l can list many files. - maxBuffer: 256 * 1024 * 1024, - windowsHide: true, - }); - const files = output - .trim() - .split('\n') - .filter((f) => f.length > 0); - - for (const file of files) { - const normalizedFile = file.replace(/\\/g, '/').replace(/^\.\//, ''); - // Never downgrade a graph-classified file to text_search. - if (!fileConfidence.has(normalizedFile)) { - fileConfidence.set(normalizedFile, 'text_search'); - } - } - } catch (e) { - logQueryError('rename:ripgrep', e); - } - - // Enumerate every `\boldName\b` line in each file to rewrite, so the previewed - // file set is exactly the set the apply loop below rewrites. A file with no - // matching line is dropped (apply would write nothing to it). `wordTest` - // (non-global) probes each line; `wordReplace` (global) rewrites it and is - // reused by the apply loop — compiled once each rather than once per line, - // and one escaping formula serves both passes. - const wordTest = new RegExp(`\\b${escapedOldName}\\b`); - const wordReplace = new RegExp(`\\b${escapedOldName}\\b`, 'g'); - const changes = new Map(); - - for (const [filePath, confidence] of fileConfidence) { - try { - const content = await fs.readFile(assertSafePath(filePath), 'utf-8'); - const lines = content.split('\n'); - const edits: RenameEdit[] = []; - for (let i = 0; i < lines.length; i++) { - if (!wordTest.test(lines[i])) { - continue; - } - edits.push({ - line: i + 1, - old_text: lines[i].trim(), - new_text: lines[i].replace(wordReplace, new_name).trim(), - confidence, - }); - } - if (edits.length > 0) { - changes.set(filePath, { file_path: filePath, edits }); - } - } catch (e) { - logQueryError('rename:enumerate', e); - } - } - - // Step 4: Apply or preview. - const failedFiles: string[] = []; - if (!dry_run) { - for (const change of changes.values()) { - try { - const fullPath = assertSafePath(change.file_path); - const content = await fs.readFile(fullPath, 'utf-8'); - await fs.writeFile(fullPath, content.replace(wordReplace, new_name), 'utf-8'); - } catch (e) { - // A swallowed write failure must not be reported as success (#2283): - // record the file so the result degrades to 'partial'. - logQueryError('rename:apply-edit', e); - failedFiles.push(change.file_path); - } - } - // A file whose write threw did not land, so drop its edits from the - // reported result — total_edits/changes must describe what actually - // reached disk, not what was attempted (#2605: the report matches reality - // even on partial failure). failed_files still names every dropped file. - for (const f of failedFiles) { - changes.delete(f); - } - } - - // Counts derive from the reported set (dry-run: every enumerated file; - // apply: only files that landed), so the graph/text_search split always - // sums to total_edits and never overstates a partial apply. - const reported = Array.from(changes.values()); - let graphEdits = 0; - let astSearchEdits = 0; - for (const change of reported) { - for (const edit of change.edits) { - if (edit.confidence === 'graph') { - graphEdits++; - } else { - astSearchEdits++; - } - } - } - - return { - status: failedFiles.length > 0 ? 'partial' : 'success', - old_name: oldName, - new_name, - files_affected: reported.length, - total_edits: graphEdits + astSearchEdits, - graph_edits: graphEdits, - text_search_edits: astSearchEdits, - changes: reported, - applied: !dry_run, - ...(failedFiles.length > 0 && { failed_files: failedFiles }), - }; + if (lookupResult.status === 'ambiguous' || lookupResult.error) return lookupResult; + // Incoming graph edges are discovery hints, not proof of source occurrences. + return renameSymbol(repo.repoPath, lookupResult.symbol, params); } private async trace(repo: RepoHandle, params: TraceParams): Promise { diff --git a/gitnexus/src/mcp/local/rename/rename-plan.ts b/gitnexus/src/mcp/local/rename/rename-plan.ts new file mode 100644 index 000000000..b8308b204 --- /dev/null +++ b/gitnexus/src/mcp/local/rename/rename-plan.ts @@ -0,0 +1,313 @@ +import fs from 'node:fs/promises'; +import path from 'node:path'; + +export interface RenameSymbol { + uid?: string; + name: string; + kind?: string; + filePath: string; + /** Graph display ranges are one-based, inclusive. */ + startLine: number; + endLine?: number; +} + +export interface RenameOptions { + new_name: string; + tsconfig_path?: string; + dry_run?: boolean; +} + +export interface RenameCoverage { + name: string; + version: string; + scope: 'project'; + tsconfig_path: string; + source_file_count: number; +} + +export interface OccurrenceEdit { + file_path: string; + /** Offsets into the unchanged source, in UTF-16 code units. */ + start: number; + length: number; + old_text: string; + new_text: string; +} + +export interface RenamePlan { + symbol: RenameSymbol; + new_name: string; + coverage: RenameCoverage; + /** Includes all source/config files read by the provider, even without edits. */ + snapshots: Map; + edits: OccurrenceEdit[]; +} + +interface ReportedEdit extends Omit { + line: number; + confidence: 'semantic'; + before: string; + after: string; +} + +interface FileChange { + file_path: string; + edits: ReportedEdit[]; +} + +export interface RenameResult { + result_version: 2; + status: 'success' | 'error' | 'partial'; + planning_status: 'ready' | 'blocked' | 'unsupported'; + application_status: 'not_requested' | 'not_started' | 'applied' | 'partial' | 'failed'; + applied: boolean; + old_name: string; + new_name: string; + files_affected: number; + total_edits: number; + semantic_edits: number; + graph_edits: 0; + text_search_edits: 0; + text_search: 'not_used'; + changes: FileChange[]; + coverage?: RenameCoverage; + error?: string; + code?: string; + candidates?: string[]; + failed_files?: string[]; +} + +export class RenameFailure extends Error { + constructor( + public readonly code: string, + message: string, + public readonly planningStatus: 'blocked' | 'unsupported' = 'blocked', + public readonly candidates?: string[], + ) { + super(message); + } +} + +/** Lexical containment must be checked before resolving symlinks. */ +export function repositoryPath(repoPath: string, filePath: string): string { + const root = path.resolve(repoPath); + const absolute = path.resolve(root, filePath); + const relative = path.relative(root, absolute); + if ( + !relative || + relative === '..' || + relative.startsWith(`..${path.sep}`) || + path.isAbsolute(relative) + ) { + throw new RenameFailure( + 'outside_repository', + `Rename path is outside the repository: ${filePath}`, + ); + } + return absolute; +} + +async function checkedRealPath(repoPath: string, filePath: string): Promise { + const absolute = repositoryPath(repoPath, filePath); + const [root, real] = await Promise.all([fs.realpath(repoPath), fs.realpath(absolute)]); + repositoryPath(root, real); + return real; +} + +function result(symbol: RenameSymbol, newName: string, changes: FileChange[] = []): RenameResult { + const count = changes.reduce((sum, file) => sum + file.edits.length, 0); + return { + result_version: 2, + status: 'success', + planning_status: 'ready', + application_status: 'not_requested', + applied: false, + old_name: symbol.name, + new_name: newName, + files_affected: changes.length, + total_edits: count, + semantic_edits: count, + graph_edits: 0, + text_search_edits: 0, + text_search: 'not_used', + changes, + }; +} + +function blocked(symbol: RenameSymbol, newName: string, error: unknown): RenameResult { + return { + ...result(symbol, newName), + status: 'error', + application_status: 'not_started', + planning_status: error instanceof RenameFailure ? error.planningStatus : 'blocked', + code: error instanceof RenameFailure ? error.code : 'rename_failed', + error: error instanceof Error ? error.message : String(error), + ...(error instanceof RenameFailure && error.candidates ? { candidates: error.candidates } : {}), + }; +} + +async function validateSnapshots(plan: RenamePlan): Promise { + for (const [file, snapshot] of plan.snapshots) { + if (!(await fs.readFile(file)).equals(Buffer.from(snapshot, 'utf8'))) { + throw new RenameFailure( + 'source_changed', + `Source changed while planning rename: ${file}. Retry from current sources.`, + ); + } + } +} + +/** Validates the complete edit set and source versions before any write. */ +export async function executeRenamePlan( + repoPath: string, + plan: RenamePlan, + dryRun = true, +): Promise { + const changes: FileChange[] = []; + const files = new Map< + string, + { absolute: string; real: string; snapshot: string; output: string } + >(); + try { + if (!plan.edits.length) + throw new RenameFailure('no_locations', 'The provider returned no rename locations.'); + const byFile = new Map(); + for (const edit of plan.edits) { + const absolute = repositoryPath(repoPath, edit.file_path); + const key = path.relative(path.resolve(repoPath), absolute).split(path.sep).join('/'); + const edits = byFile.get(key) ?? []; + edits.push(edit); + byFile.set(key, edits); + } + const realPaths = new Set(); + for (const [file, unsorted] of [...byFile.entries()].sort(([a], [b]) => a.localeCompare(b))) { + const absolute = repositoryPath(repoPath, file); + const real = await checkedRealPath(repoPath, file); + if (realPaths.has(real)) + throw new RenameFailure( + 'duplicate_file', + `Multiple rename paths refer to the same file: ${file}`, + ); + realPaths.add(real); + const snapshot = plan.snapshots.get(absolute); + if (snapshot === undefined) + throw new RenameFailure('missing_snapshot', `No source snapshot for ${file}.`); + const edits: OccurrenceEdit[] = []; + for (const edit of unsorted.sort((a, b) => a.start - b.start || a.length - b.length)) { + if ( + !Number.isSafeInteger(edit.start) || + !Number.isSafeInteger(edit.length) || + edit.start < 0 || + edit.length <= 0 || + edit.start + edit.length > snapshot.length || + snapshot.slice(edit.start, edit.start + edit.length) !== edit.old_text + ) { + throw new RenameFailure( + 'invalid_edit', + `Invalid rename span or expected text in ${file}.`, + ); + } + const previous = edits.at(-1); + if (previous && previous.start + previous.length > edit.start) { + if ( + previous.start === edit.start && + previous.length === edit.length && + previous.old_text === edit.old_text && + previous.new_text === edit.new_text + ) + continue; + throw new RenameFailure('overlapping_edits', `Conflicting rename spans in ${file}.`); + } + edits.push(edit); + } + let output = snapshot; + for (const edit of [...edits].reverse()) + output = + output.slice(0, edit.start) + edit.new_text + output.slice(edit.start + edit.length); + files.set(file, { absolute, real, snapshot, output }); + changes.push({ + file_path: file, + edits: edits.map(({ file_path: _file, ...edit }) => { + const lineStart = snapshot.lastIndexOf('\n', edit.start - 1) + 1; + const nextLine = snapshot.indexOf('\n', edit.start); + const lineEnd = nextLine < 0 ? snapshot.length : nextLine; + return { + ...edit, + confidence: 'semantic', + line: snapshot.slice(0, edit.start).split('\n').length, + before: snapshot.slice(lineStart, lineEnd), + after: + snapshot.slice(lineStart, edit.start) + + edit.new_text + + snapshot.slice(edit.start + edit.length, lineEnd), + }; + }), + }); + } + await validateSnapshots(plan); + } catch (error) { + return { ...blocked(plan.symbol, plan.new_name, error), coverage: plan.coverage }; + } + + if (dryRun) return { ...result(plan.symbol, plan.new_name, changes), coverage: plan.coverage }; + const landed: FileChange[] = []; + for (const change of changes) { + const file = files.get(change.file_path)!; + try { + // Recheck each destination immediately before its write. A concurrent edit + // after an earlier write is a partial operation, never a successful rename. + if ( + (await checkedRealPath(repoPath, change.file_path)) !== file.real || + !(await fs.readFile(file.absolute)).equals(Buffer.from(file.snapshot, 'utf8')) + ) { + throw new RenameFailure( + 'source_changed', + `Source changed before writing ${change.file_path}.`, + ); + } + await fs.writeFile(file.absolute, file.output, 'utf8'); + landed.push(change); + } catch (error) { + return { + ...result(plan.symbol, plan.new_name, landed), + coverage: plan.coverage, + status: 'partial', + applied: landed.length > 0, + application_status: landed.length ? 'partial' : 'failed', + code: error instanceof RenameFailure ? error.code : 'write_failed', + error: `${error instanceof Error ? error.message : String(error)} Check failed files before retrying; writes are not transactional.`, + failed_files: changes.slice(landed.length).map((c) => c.file_path), + }; + } + } + return { + ...result(plan.symbol, plan.new_name, landed), + coverage: plan.coverage, + applied: true, + application_status: 'applied', + }; +} + +/** One planner for preview and application; unsupported providers never fall back to text search. */ +export async function renameSymbol( + repoPath: string, + symbol: RenameSymbol, + options: RenameOptions, +): Promise { + try { + if (!/\.(?:[cm]?[jt]s|[jt]sx)$/i.test(symbol.filePath)) { + throw new RenameFailure( + 'unsupported_language', + 'Semantic rename currently supports configured TypeScript and JavaScript projects only.', + 'unsupported', + ); + } + if (options.dry_run !== undefined && typeof options.dry_run !== 'boolean') + throw new RenameFailure('invalid_argument', 'dry_run must be a boolean.'); + const { planTypeScriptRename } = await import('./typescript-rename.js'); + const plan = planTypeScriptRename(repoPath, symbol, options); + return await executeRenamePlan(repoPath, plan, options.dry_run ?? true); + } catch (error) { + return blocked(symbol, options.new_name, error); + } +} diff --git a/gitnexus/src/mcp/local/rename/typescript-rename.ts b/gitnexus/src/mcp/local/rename/typescript-rename.ts new file mode 100644 index 000000000..6fe582eff --- /dev/null +++ b/gitnexus/src/mcp/local/rename/typescript-rename.ts @@ -0,0 +1,359 @@ +import fs from 'node:fs'; +import path from 'node:path'; +import ts from 'typescript-language-service'; +import { + RenameFailure, + repositoryPath, + type RenameOptions, + type RenamePlan, + type RenameSymbol, +} from './rename-plan.js'; + +function checkNewName(name: string, oldName: string): void { + if (typeof name !== 'string' || !name || name === oldName || name.includes('\\')) { + throw new RenameFailure('invalid_name', 'new_name must be a different, unescaped identifier.'); + } + const scanner = ts.createScanner( + ts.ScriptTarget.Latest, + false, + ts.LanguageVariant.Standard, + name, + ); + if ( + scanner.scan() !== ts.SyntaxKind.Identifier || + scanner.getTokenPos() !== 0 || + scanner.getTextPos() !== name.length || + scanner.scan() !== ts.SyntaxKind.EndOfFileToken + ) { + throw new RenameFailure( + 'invalid_name', + 'new_name must be one identifier; keywords, whitespace and punctuation are unsupported.', + ); + } +} + +function declarationKind(node: ts.Node): string | undefined { + if ( + ts.isMethodDeclaration(node) || + ts.isMethodSignature(node) || + ts.isGetAccessor(node) || + ts.isSetAccessor(node) + ) + return 'method'; + if (ts.isFunctionDeclaration(node) || ts.isFunctionExpression(node)) return 'function'; + if (ts.isClassDeclaration(node) || ts.isClassExpression(node)) return 'class'; + if (ts.isInterfaceDeclaration(node)) return 'interface'; + if (ts.isTypeAliasDeclaration(node)) return 'type'; + if (ts.isEnumDeclaration(node)) return 'enum'; + if (ts.isModuleDeclaration(node)) return 'namespace'; + if ( + ts.isPropertyDeclaration(node) || + ts.isPropertySignature(node) || + ts.isPropertyAssignment(node) + ) + return 'property'; + if (ts.isVariableDeclaration(node)) + return node.initializer && + (ts.isArrowFunction(node.initializer) || ts.isFunctionExpression(node.initializer)) + ? 'function' + : 'variable'; + if (ts.isParameter(node) || ts.isBindingElement(node)) return 'variable'; + return undefined; +} + +function matchesKind(actual: string, expected?: string): boolean { + if (!expected) return true; + const kind = expected.toLowerCase(); + if (kind === 'const' || kind === 'variable') return actual === 'variable'; + if (kind === 'typealias') return actual === 'type'; + return actual === kind; +} + +function ownerName(node: ts.Node): string { + const owners: string[] = []; + for (let parent = node.parent; parent && !ts.isSourceFile(parent); parent = parent.parent) { + if (declarationKind(parent)) { + const name = (parent as ts.NamedDeclaration).name; + if (name && ts.isIdentifier(name)) owners.unshift(name.text); + } + } + return owners.join('.'); +} + +function declarationPosition(source: ts.SourceFile, symbol: RenameSymbol): number { + if ( + !Number.isSafeInteger(symbol.startLine) || + symbol.startLine < 1 || + (symbol.endLine !== undefined && + (!Number.isSafeInteger(symbol.endLine) || symbol.endLine < symbol.startLine)) + ) { + throw new RenameFailure( + 'stale_symbol', + 'The indexed declaration has no usable source range. Reindex and select it again.', + ); + } + let expectedOwner: string | undefined; + if (symbol.uid) { + const prefix = `${symbol.kind ?? symbol.uid.split(':')[0]}:${symbol.filePath}:`; + if (!symbol.uid.startsWith(prefix)) + throw new RenameFailure( + 'stale_symbol', + 'Symbol identity does not match its file and kind. Reindex and select it again.', + ); + const qualified = symbol.uid.slice(prefix.length).replace(/#\d+.*$/, ''); + if (qualified === symbol.name) expectedOwner = ''; + else if (qualified.endsWith(`.${symbol.name}`)) + expectedOwner = qualified.slice(0, -(symbol.name.length + 1)); + else + throw new RenameFailure( + 'stale_symbol', + 'This indexed declaration identity is not supported. Reindex and select a named declaration.', + ); + } + const candidates: ts.Identifier[] = []; + function visit(node: ts.Node): void { + const kind = declarationKind(node); + const name = kind ? (node as ts.NamedDeclaration).name : undefined; + if ( + name && + ts.isIdentifier(name) && + name.text === symbol.name && + matchesKind(kind!, symbol.kind) + ) { + // Tree-sitter records variable statements (including export/const), while + // the compiler owns the identifier on a VariableDeclaration. + const rangeNode = + ts.isVariableDeclaration(node) && + ts.isVariableDeclarationList(node.parent) && + ts.isVariableStatement(node.parent.parent) + ? node.parent.parent + : node; + const start = source.getLineAndCharacterOfPosition(rangeNode.getStart(source)).line + 1; + const end = + source.getLineAndCharacterOfPosition( + Math.max(rangeNode.getStart(source), rangeNode.end - 1), + ).line + 1; + if ( + start === symbol.startLine && + (symbol.endLine === undefined || end === symbol.endLine) && + (expectedOwner === undefined || ownerName(node) === expectedOwner) + ) + candidates.push(name); + } + ts.forEachChild(node, visit); + } + visit(source); + if (candidates.length !== 1) + throw new RenameFailure( + 'stale_symbol', + 'The indexed name, kind, owner and range do not identify one current declaration. Reindex and select it again.', + ); + return candidates[0].getStart(source); +} + +/** Uses our pinned compiler API only; project compiler packages/plugins are never executed. */ +export function planTypeScriptRename( + repoPath: string, + symbol: RenameSymbol, + options: RenameOptions, +): RenamePlan { + checkNewName(options.new_name, symbol.name); + const root = path.resolve(repoPath); + const target = repositoryPath(root, symbol.filePath); + const realRoot = fs.realpathSync(root); + repositoryPath(realRoot, fs.realpathSync(target)); + const snapshots = new Map(); + const readFile = (filename: string): string | undefined => { + const absolute = path.resolve(filename); + if (snapshots.has(absolute)) return snapshots.get(absolute); + try { + // Keep the exact on-disk BOM and UTF-16 offsets; ts.sys.readFile strips BOMs. + const bytes = fs.readFileSync(absolute); + const text = bytes.toString('utf8'); + if (!Buffer.from(text, 'utf8').equals(bytes)) + throw new RenameFailure( + 'unsupported_encoding', + `Rename requires losslessly decoded UTF-8 input: ${absolute}`, + 'unsupported', + ); + snapshots.set(absolute, text); + return text; + } catch (error) { + if (error instanceof RenameFailure) throw error; + if ((error as NodeJS.ErrnoException).code === 'ENOENT') return undefined; + throw new RenameFailure( + 'read_failed', + `Cannot read rename input ${absolute}: ${error instanceof Error ? error.message : String(error)}`, + ); + } + }; + const canonical = (file: string) => + ts.sys.useCaseSensitiveFileNames ? path.resolve(file) : path.resolve(file).toLowerCase(); + let configs: string[] = []; + if (options.tsconfig_path !== undefined) { + if ( + typeof options.tsconfig_path !== 'string' || + !options.tsconfig_path || + path.isAbsolute(options.tsconfig_path) + ) { + throw new RenameFailure( + 'invalid_config_path', + 'tsconfig_path must be a repository-relative config filename.', + ); + } + configs = [repositoryPath(root, options.tsconfig_path)]; + } else { + for (let dir = path.dirname(target); ; dir = path.dirname(dir)) { + configs.push( + ...fs + .readdirSync(dir) + .filter((name) => /^(?:tsconfig|jsconfig).*\.json$/i.test(name)) + .map((name) => path.join(dir, name)), + ); + if (dir === root) break; + } + } + if (!configs.length) + throw new RenameFailure( + 'missing_project', + 'No configured TS/JS project found. Add a config or supply tsconfig_path.', + 'unsupported', + ); + const candidates: { config: string; parsed: ts.ParsedCommandLine }[] = []; + for (const config of configs.sort()) { + repositoryPath(realRoot, fs.realpathSync(config)); + const loaded = ts.readConfigFile(config, readFile); + const parsed = loaded.error + ? undefined + : ts.parseJsonConfigFileContent( + loaded.config, + { ...ts.sys, readFile }, + path.dirname(config), + undefined, + config, + ); + const errors = loaded.error ? [loaded.error] : parsed!.errors; + if (errors.length) + throw new RenameFailure( + 'invalid_project', + `Cannot use ${path.relative(root, config)}: ${errors.map((e) => ts.flattenDiagnosticMessageText(e.messageText, ' ')).join('; ')}`, + 'unsupported', + ); + if (parsed!.fileNames.some((file) => canonical(file) === canonical(target))) + candidates.push({ config, parsed: parsed! }); + } + if (!candidates.length) + throw new RenameFailure( + 'missing_project', + 'The selected declaration is not included in a configured project. Supply tsconfig_path for its owning project.', + 'unsupported', + ); + if (candidates.length > 1) + throw new RenameFailure( + 'ambiguous_project', + 'Several configured projects contain this declaration. Select one using tsconfig_path.', + 'blocked', + candidates.map((c) => path.relative(root, c.config).split(path.sep).join('/')), + ); + const { config, parsed } = candidates[0]; + if (parsed.projectReferences?.length) + throw new RenameFailure( + 'project_references', + 'Project references require cross-project coordination and are not supported for rename. Select a self-contained project.', + 'unsupported', + ); + const host: ts.LanguageServiceHost = { + ...ts.sys, + readFile, + getCurrentDirectory: () => root, + useCaseSensitiveFileNames: () => ts.sys.useCaseSensitiveFileNames, + getCompilationSettings: () => parsed.options, + getScriptFileNames: () => parsed.fileNames, + getScriptVersion: () => '0', + getScriptSnapshot: (filename) => { + const text = readFile(filename); + return text === undefined ? undefined : ts.ScriptSnapshot.fromString(text); + }, + getDefaultLibFileName: (compilerOptions) => ts.getDefaultLibFilePath(compilerOptions), + }; + const service = ts.createLanguageService(host); + try { + const program = service.getProgram(); + const source = program?.getSourceFile(target); + if (!source) + throw new RenameFailure( + 'missing_source', + 'The compiler could not load the indexed source.', + 'unsupported', + ); + // Capture every source file, including unedited dependencies: a changed + // declaration elsewhere can invalidate the compiler's binding decisions. + for (const file of program.getSourceFiles()) { + if (readFile(file.fileName) !== file.text) + throw new RenameFailure( + 'source_changed', + `Compiler source snapshot mismatch: ${file.fileName}`, + ); + } + const position = declarationPosition(source, symbol); + const preferences: ts.UserPreferences = { + providePrefixAndSuffixTextForRename: true, + allowRenameOfImportPath: false, + }; + const info = service.getRenameInfo(target, position, preferences); + if (info.canRename === false) + throw new RenameFailure('unsupported_target', info.localizedErrorMessage, 'unsupported'); + if (info.fileToRename) + throw new RenameFailure( + 'file_rename', + 'File and import-path renames are not supported.', + 'unsupported', + ); + const locations = service.findRenameLocations(target, position, false, false, preferences); + if (!locations?.length) + throw new RenameFailure( + 'no_locations', + 'The compiler returned no rename locations.', + 'unsupported', + ); + return { + symbol, + new_name: options.new_name, + snapshots, + coverage: { + name: 'typescript', + version: ts.version, + scope: 'project', + tsconfig_path: path.relative(root, config).split(path.sep).join('/'), + source_file_count: program + .getSourceFiles() + .filter( + (file) => + !program.isSourceFileDefaultLibrary(file) && + !program.isSourceFileFromExternalLibrary(file), + ).length, + }, + edits: locations.map((location) => { + const absolute = repositoryPath(root, location.fileName); + const snapshot = snapshots.get(absolute); + if (snapshot === undefined) + throw new RenameFailure( + 'missing_snapshot', + `The compiler returned an uncaptured source: ${location.fileName}`, + ); + return { + file_path: path.relative(root, absolute).split(path.sep).join('/'), + start: location.textSpan.start, + length: location.textSpan.length, + old_text: snapshot.slice( + location.textSpan.start, + location.textSpan.start + location.textSpan.length, + ), + new_text: `${location.prefixText ?? ''}${options.new_name}${location.suffixText ?? ''}`, + }; + }), + }; + } finally { + service.dispose(); + } +} diff --git a/gitnexus/src/mcp/read-only-policy.ts b/gitnexus/src/mcp/read-only-policy.ts index 24c550cca..107b1ce59 100644 --- a/gitnexus/src/mcp/read-only-policy.ts +++ b/gitnexus/src/mcp/read-only-policy.ts @@ -10,6 +10,7 @@ export const MCP_READ_ONLY_TOOLS = new Set([ 'grep', 'detect_changes', 'check', + 'rename_preview', 'impact', 'explain', 'pdg_query', diff --git a/gitnexus/src/mcp/resources.ts b/gitnexus/src/mcp/resources.ts index 49687fa9b..1e2d4c3f8 100644 --- a/gitnexus/src/mcp/resources.ts +++ b/gitnexus/src/mcp/resources.ts @@ -415,7 +415,10 @@ async function getContextResource(backend: LocalBackend, repoName?: string): Pro ' - explain: Persisted taint findings — source→sink data flows with per-hop variables (requires analyze --pdg)', ); lines.push(' - detect_changes: Git-diff impact analysis (what do your changes affect)'); - lines.push(' - rename: Multi-file coordinated rename with confidence tags'); + lines.push( + ' - rename_preview: Read-only semantic rename preview for a configured TS/JS project', + ); + lines.push(' - rename: Preview or apply exact semantic occurrences in the selected project'); lines.push(' - cypher: Raw graph queries'); lines.push(' - list_repos: Discover all indexed repositories'); lines.push(''); @@ -700,7 +703,8 @@ async function getSetupResource(backend: LocalBackend): Promise { '| `context` | 360-degree symbol view — categorized refs, processes it participates in |', '| `impact` | Symbol blast radius — what breaks at depth 1/2/3 with confidence |', '| `detect_changes` | Git-diff impact — what do your current changes affect |', - '| `rename` | Multi-file coordinated rename with confidence-tagged edits |', + '| `rename_preview` | Read-only exact-span rename preview for a configured TS/JS project |', + '| `rename` | Preview or apply semantic rename in the selected project |', '| `cypher` | Raw graph queries |', '| `list_repos` | Discover indexed repos |', '', diff --git a/gitnexus/src/mcp/tools.ts b/gitnexus/src/mcp/tools.ts index 77b527e00..948a44539 100644 --- a/gitnexus/src/mcp/tools.ts +++ b/gitnexus/src/mcp/tools.ts @@ -103,6 +103,18 @@ const MUTATING_REPO_OMISSION = const HOT_READ_STALENESS_NOTE = "Object results attach `staleness` even when current. Read `staleness.branch`/`lastCommit` for which index answered and `status` for freshness. Re-analyze only for `behind` or `diverged` — `current` is this clone's HEAD, not necessarily the default branch; `unknown` is unmeasurable, not stale. Field is only on object results (not raw-array cypher, error envelopes, or `@group` calls)."; +const RENAME_PROPERTIES: ToolDefinition['inputSchema']['properties'] = { + symbol_name: { type: 'string', description: 'Current symbol name to rename' }, + symbol_uid: { type: 'string', description: 'Direct symbol UID from prior tool results' }, + new_name: { type: 'string', description: 'The new identifier for the symbol' }, + file_path: { type: 'string', description: 'File path to disambiguate common names' }, + tsconfig_path: { + type: 'string', + description: + 'Repository-relative TypeScript/JavaScript project config; required when project selection is ambiguous', + }, +}; + export const GITNEXUS_TOOLS: ToolDefinition[] = [ { name: 'list_repos', @@ -468,39 +480,42 @@ A graph too large to analyze at all returns \`{ error, truncated: true }\` with required: [], }, }, + { + name: 'rename_preview', + description: `Read-only semantic rename preview for one configured TypeScript/JavaScript project in the current checkout. +Uses the TypeScript language service to report exact UTF-16 spans, including import/export alias affixes. Comments, strings, and unrelated same-spelled symbols are not text-search replacements. +Pass symbol_uid from context() to select the declaration. If multiple project configs include it, select a returned candidate with tsconfig_path. Unsupported languages, project references, and unresolved declarations fail closed. +Returns result_version:2, semantic_edits, and coverage identifying the selected project; coverage is not repository-wide. This tool cannot apply edits.`, + annotations: READ_ONLY_TOOL_ANNOTATIONS, + inputSchema: { + type: 'object', + properties: { + ...RENAME_PROPERTIES, + repo: { + type: 'string', + description: `Repository name or path. ${CWD_AWARE_REPO_OMISSION}`, + }, + }, + required: ['new_name'], + }, + }, { name: 'rename', - description: `Multi-file coordinated rename using the knowledge graph + text search. -Finds all references via graph (high confidence) and regex text search (lower confidence). Preview by default. - -WHEN TO USE: Renaming a function, class, method, or variable across the codebase. Safer than find-and-replace. -AFTER THIS: Run detect_changes() to verify no unexpected side effects. - -Each edit is tagged with confidence: -- "graph": found via knowledge graph relationships (high confidence, safe to accept) -- "text_search": found via regex text search (lower confidence, review carefully) - -Handles disambiguation via context()'s payload verbatim: an ambiguous symbol_name returns status "ambiguous" with ranked candidates and totalCandidates — the TRUE match count, not candidates[].length — plus candidatesTruncated:true and a "(showing M)" suffix on message when candidates[] is the shorter window. Re-call with symbol_uid.`, + description: `Semantic rename for one configured TypeScript/JavaScript project in the current checkout. Previews by default; dry_run:false applies exact compiler-selected occurrences after checking source snapshots. +Use rename_preview for read-only clients. Pass symbol_uid from context() to select the declaration and tsconfig_path when project selection is ambiguous. Unsupported languages, project references, and unresolved declarations fail closed; there is no text-search fallback. +Returns result_version:2 with semantic_edits (occurrence count), exact UTF-16 spans, project coverage, and explicit planning/application status. graph_edits and text_search_edits are zero. Partial writes report only landed edits and failed_files; review failures before retrying. Separate preview/apply calls recompute the plan. +Ambiguous symbol_name returns context()'s ranked candidates and totalCandidates verbatim; re-call with symbol_uid. After application, run tests and detect_changes().`, annotations: DESTRUCTIVE_TOOL_ANNOTATIONS, inputSchema: { type: 'object', properties: { - symbol_name: { type: 'string', description: 'Current symbol name to rename' }, - symbol_uid: { - type: 'string', - description: 'Direct symbol UID from prior tool results (zero-ambiguity)', - }, - new_name: { type: 'string', description: 'The new name for the symbol' }, - file_path: { type: 'string', description: 'File path to disambiguate common names' }, + ...RENAME_PROPERTIES, dry_run: { type: 'boolean', description: 'Preview edits without modifying files (default: true)', default: true, }, - repo: { - type: 'string', - description: `Repository name or path. ${MUTATING_REPO_OMISSION}`, - }, + repo: { type: 'string', description: `Repository name or path. ${MUTATING_REPO_OMISSION}` }, }, required: ['new_name'], }, @@ -1122,7 +1137,7 @@ Optional caseSensitive and literal match HTTP /api/grep (default: case-insensiti * loop below skips `branch` for `CHECKOUT_SOURCE_TOOLS` — a pin would label * checkout bytes with another commit. */ -export const CHECKOUT_SOURCE_TOOLS = new Set(['read_file', 'grep']); +export const CHECKOUT_SOURCE_TOOLS = new Set(['read_file', 'grep', 'rename', 'rename_preview']); export const REPO_SCOPED_TOOLS = new Set([ 'read_file', @@ -1136,6 +1151,7 @@ export const REPO_SCOPED_TOOLS = new Set([ 'check', 'impact', 'rename', + 'rename_preview', 'route_map', 'tool_map', 'shape_check', diff --git a/gitnexus/test/unit/calltool-dispatch.test.ts b/gitnexus/test/unit/calltool-dispatch.test.ts index aa4bc9d82..111186e71 100644 --- a/gitnexus/test/unit/calltool-dispatch.test.ts +++ b/gitnexus/test/unit/calltool-dispatch.test.ts @@ -2534,20 +2534,31 @@ describe('LocalBackend.callTool', () => { expect(result.error).toContain('Either symbol_name or symbol_uid'); }); + it('dispatches rename_preview as a forced dry run', async () => { + const rename = vi.spyOn(backend as any, 'rename').mockResolvedValue({ applied: false }); + const result = await backend.callTool('rename_preview', { new_name: 'newName' }); + expect(result.applied).toBe(false); + expect(rename).toHaveBeenCalledWith(expect.anything(), { new_name: 'newName', dry_run: true }); + }); + + it.each(['rename', 'rename_preview'])('%s rejects branch-pinned checkout edits', async (tool) => { + const rename = vi.spyOn(backend as any, 'rename'); + const result = await backend.callTool(tool, { new_name: 'newName', branch: 'main' }); + expect(result.error).toMatch(/branch/); + expect(rename).not.toHaveBeenCalled(); + }); + it('rename: a swallowed apply-edit write failure degrades to status:partial + failed_files (#2283)', async () => { - // Resolve the definition, no graph refs. readFile succeeds (so a def edit is - // recorded), but writeFile fails on apply — the failure is swallowed via - // logQueryError. The result must NOT report a clean success: it degrades to - // 'partial' and lists the unwritten file, instead of status:'success'. + // A real configured project produces an edit; only its final write fails. (executeParameterized as any) .mockResolvedValueOnce([ { - id: 'func:oldName', + id: 'Function:src/target.ts:oldName', name: 'oldName', type: 'Function', filePath: 'src/target.ts', startLine: 1, - endLine: 5, + endLine: 1, }, ]) .mockResolvedValue([]); @@ -2558,12 +2569,10 @@ describe('LocalBackend.callTool', () => { backend = new LocalBackend(); await backend.init(); - // The symbol is stored at 0-based startLine 1; context() presents it 1-based - // (line 2) and rename subtracts 1 to recover the 0-based file index (1), so - // `oldName` must sit on the file's 0-based line 1 for the definition edit to - // fire. (#2380: the mock previously put it on line 0, which stopped matching - // once context() went 1-based.) - const readSpy = vi.spyOn(fsPromises, 'readFile').mockResolvedValue('\nfunction oldName() {}\n'); + // Graph lines are zero-based; context presents this declaration on line 2. + mkdirSync(path.join(repoDir, 'src')); + writeFileSync(path.join(repoDir, 'src/target.ts'), '\nfunction oldName() {}\n'); + writeFileSync(path.join(repoDir, 'tsconfig.json'), '{"include":["src/**/*.ts"]}'); const writeSpy = vi .spyOn(fsPromises, 'writeFile') .mockRejectedValue(new Error('EACCES: permission denied')); @@ -2575,11 +2584,10 @@ describe('LocalBackend.callTool', () => { }); expect(result.status).toBe('partial'); expect(result.failed_files).toContain('src/target.ts'); - // It DID attempt to apply (not a dry run) — `applied` stays true; the - // honest signal is the 'partial' status + failed_files, not `applied`. - expect(result.applied).toBe(true); + expect(result.applied).toBe(false); + expect(result.total_edits).toBe(0); + expect(result.application_status).toBe('failed'); } finally { - readSpy.mockRestore(); writeSpy.mockRestore(); rmSync(repoDir, { recursive: true, force: true }); } diff --git a/gitnexus/test/unit/mcp-read-only.test.ts b/gitnexus/test/unit/mcp-read-only.test.ts index 07d01a567..b41ab9697 100644 --- a/gitnexus/test/unit/mcp-read-only.test.ts +++ b/gitnexus/test/unit/mcp-read-only.test.ts @@ -16,6 +16,7 @@ const READ_ONLY_TOOLS = [ 'pdg_query', 'query', 'read_file', + 'rename_preview', 'route_map', 'shape_check', 'tool_map', @@ -65,6 +66,34 @@ afterEach(() => { }); describe('MCP read-only mode', () => { + it('offers a read-only rename preview with no mutation or branch arguments', async () => { + enableReadOnly(); + const session = await connect(); + try { + const { tools } = await session.client.listTools(); + const preview = tools.find((tool) => tool.name === 'rename_preview'); + expect(preview?.annotations).toMatchObject({ readOnlyHint: true, destructiveHint: false }); + expect(preview?.inputSchema.properties).not.toHaveProperty('dry_run'); + expect(preview?.inputSchema.properties).not.toHaveProperty('branch'); + await session.client.callTool({ + name: 'rename_preview', + arguments: { symbol_uid: 'Method:src/writer.ts:Writer.close#0', new_name: 'closeWriter' }, + }); + expect(session.backend.callTool).toHaveBeenCalledWith('rename_preview', { + symbol_uid: 'Method:src/writer.ts:Writer.close#0', + new_name: 'closeWriter', + }); + const invalid = await session.client.callTool({ + name: 'rename_preview', + arguments: { new_name: 'closeWriter', dry_run: false }, + }); + expect(invalid.isError).toBe(true); + expect(session.backend.callTool).toHaveBeenCalledTimes(1); + } finally { + await session.close(); + } + }); + it('discovers only proven single-repository read tools', async () => { enableReadOnly(); const session = await connect(); @@ -237,7 +266,8 @@ describe('MCP read-only mode', () => { for (const uri of ['gitnexus://setup', 'gitnexus://repo/test/context']) { const resource = await session.client.readResource({ uri }); const text = (resource.contents[0] as { text: string }).text; - expect(text).not.toMatch(/(?:^\s*-\s+|^\|\s*`)(?:rename|cypher)/mu); + expect(text).not.toMatch(/(?:^\s*-\s+|^\|\s*`)(?:rename|cypher)(?::|`)/mu); + expect(text).toContain('rename_preview'); expect(text).not.toContain('gitnexus://group/'); } } finally { diff --git a/gitnexus/test/unit/rename-edit-report.test.ts b/gitnexus/test/unit/rename-edit-report.test.ts index b7088b9e7..d36b5bc10 100644 --- a/gitnexus/test/unit/rename-edit-report.test.ts +++ b/gitnexus/test/unit/rename-edit-report.test.ts @@ -1,25 +1,10 @@ -/** - * Regression test for issue #2605: `rename` must report every edit it applies. - * - * The apply step does a whole-file `\boldName\b` global replace on each touched - * file, but the reported `changes`/`total_edits` were built from a partial - * enumeration that (a) recorded only the definition line, (b) recorded one edit - * per graph-ref file then broke, and (c) skipped text-search on any file already - * covered by the graph. When a private symbol's definition and all its call - * sites live in one file, only the definition line was reported (total_edits: 1) - * while apply rewrote every occurrence. These tests drive the single-file repro, - * a mixed graph/text_search multi-file rename, and a partial write failure, and - * assert the report matches what apply actually writes in each case. - */ +/** Backend regressions for #3486; retain preview/apply fidelity from #2605. */ import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; import { promises as fs } from 'node:fs'; import fsPromises from 'fs/promises'; import os from 'node:os'; import path from 'node:path'; -// Prevent onnxruntime / native search adapters from loading at import time -// (mirrors test/unit/calltool-dispatch.test.ts). We drive the private rename() -// directly, so the graph/DB/embedding layers are never exercised. vi.mock('../../src/core/search/bm25-index.js', () => ({ searchFTSFromLbug: vi.fn().mockResolvedValue({ results: [], ftsAvailable: true }), })); @@ -28,111 +13,81 @@ vi.mock('../../src/mcp/core/embedder.js', () => ({ getEmbeddingDims: vi.fn().mockReturnValue(384), })); -// rename() shells out to `rg -l` to discover text-search files. Stub it so the -// ripgrep-discovery branch is deterministic and driveable (rg is not reliably -// on PATH inside the vitest worker). Default: no hits. -const { execFileSyncMock } = vi.hoisted(() => ({ execFileSyncMock: vi.fn(() => '') })); -vi.mock('child_process', async (importActual) => { - const actual = await importActual(); - return { ...actual, execFileSync: execFileSyncMock }; -}); +// No text discovery is permitted, even when rg is installed on the host. +const { execFileSyncMock } = vi.hoisted(() => ({ + execFileSyncMock: vi.fn(() => { + throw new Error('rg unavailable'); + }), +})); +vi.mock('child_process', async (importActual) => ({ + ...(await importActual()), + execFileSync: execFileSyncMock, +})); import { LocalBackend } from '../../src/mcp/local/local-backend.js'; -type Incoming = { - calls: { filePath: string }[]; - imports: { filePath: string }[]; - extends: { filePath: string }[]; - implements: { filePath: string }[]; -}; -const EMPTY_INCOMING: Incoming = { calls: [], imports: [], extends: [], implements: [] }; - -type RenameResult = { - status: string; - applied: boolean; - files_affected: number; - total_edits: number; - graph_edits: number; - text_search_edits: number; - changes: { file_path: string; edits: { line: number; confidence: string }[] }[]; - failed_files?: string[]; -}; - -// The #2605 repro: a private free fn with exactly 4 textual occurrences of -// `rename_target` — the definition, one production call, two test calls — all -// in the same file. -const RUST_SRC = `fn rename_target(x: u32) -> u32 { - x + 1 +const SOURCE = `export class Writer { + close() {} } - -pub fn prod_call() -> u32 { - rename_target(1) +export class Other { + close() {} } - -#[cfg(test)] -mod tests { - use super::*; - - #[test] - fn unit_one() { - assert_eq!(rename_target(1), 2); - } - - #[test] - fn unit_two() { - assert_eq!(rename_target(2), 3); - } +export function run(writer: Writer, other: Other) { + const hooks = { close: () => writer.close() }; writer.close(); other.close(); + // close should remain in this comment. + const title = 'close'; + return { hooks, title }; } `; +const CALLER = `import { Writer } from './writer.js'; +export function caller(writer: Writer) { writer.close(); } +`; -/** 1-based occurrence lines of `rename_target` in `src` — the ground truth the - * report must match. Computed (not hardcoded) so editing a fixture cannot - * silently desync the expectation. */ -function occurrenceLines(src: string): number[] { - return src - .split('\n') - .map((line, i) => (/\brename_target\b/.test(line) ? i + 1 : 0)) - .filter((n) => n > 0); -} -const OCCURRENCE_LINES = occurrenceLines(RUST_SRC); - -/** Build a backend whose graph lookup returns the symbol (definition at - * src/lib.rs) with the given incoming refs. */ -function stubbedBackend(incoming: Incoming = EMPTY_INCOMING): LocalBackend { +function stubbedBackend(symbol: Record = {}) { const backend = new LocalBackend(); - vi.spyOn( - backend as unknown as { ensureInitialized: () => Promise }, - 'ensureInitialized', - ).mockResolvedValue(undefined); - vi.spyOn(backend as unknown as { context: () => Promise }, 'context').mockResolvedValue({ - status: 'success', - symbol: { name: 'rename_target', filePath: 'src/lib.rs', startLine: OCCURRENCE_LINES[0] }, - incoming, + vi.spyOn(backend as any, 'ensureInitialized').mockResolvedValue(undefined); + vi.spyOn(backend as any, 'context').mockResolvedValue({ + status: 'found', + symbol: { + uid: 'Method:src/writer.ts:Writer.close#0', + name: 'close', + kind: 'Method', + filePath: 'src/writer.ts', + startLine: 2, + endLine: 2, + ...symbol, + }, + incoming: { calls: [], imports: [], extends: [], implements: [] }, }); return backend; } -function callRename( - backend: LocalBackend, - repoPath: string, - params: Record, -): Promise { - return ( - backend as unknown as { rename: (r: unknown, p: unknown) => Promise } - ).rename({ repoPath }, { symbol_name: 'rename_target', new_name: 'renamed_fn', ...params }); -} +const callRename = (backend: LocalBackend, repoPath: string, params = {}) => + (backend as any).rename( + { repoPath }, + { + symbol_name: 'close', + new_name: 'closeWriter', + ...params, + }, + ); -const editsFor = (r: RenameResult, file: string) => - r.changes.find((c) => c.file_path === file)?.edits ?? []; - -describe('rename edit report is faithful to apply (#2605)', () => { +describe('semantic rename reports exact applied occurrences (#3486, #2605)', () => { let tmpDir: string; beforeEach(async () => { - execFileSyncMock.mockReturnValue(''); // default: no ripgrep hits - tmpDir = await fs.mkdtemp(path.join(os.tmpdir(), 'gn-2605-')); + execFileSyncMock.mockClear(); + tmpDir = await fs.mkdtemp(path.join(os.tmpdir(), 'gn-3486-backend-')); await fs.mkdir(path.join(tmpDir, 'src')); - await fs.writeFile(path.join(tmpDir, 'src', 'lib.rs'), RUST_SRC, 'utf-8'); + await fs.writeFile( + path.join(tmpDir, 'tsconfig.json'), + JSON.stringify({ + compilerOptions: { target: 'ES2022', module: 'NodeNext', moduleResolution: 'NodeNext' }, + include: ['src/**/*.ts'], + }), + ); + await fs.writeFile(path.join(tmpDir, 'src/writer.ts'), SOURCE); + await fs.writeFile(path.join(tmpDir, 'src/caller.ts'), CALLER); }); afterEach(async () => { @@ -140,100 +95,86 @@ describe('rename edit report is faithful to apply (#2605)', () => { await fs.rm(tmpDir, { recursive: true, force: true }); }); - it('previews every occurrence that apply will rewrite (dry_run)', async () => { - const result = await callRename(stubbedBackend(), tmpDir, { dry_run: true }); - - expect(result.applied).toBe(false); - expect(result.files_affected).toBe(1); - expect(result.total_edits).toBe(OCCURRENCE_LINES.length); // 4, not 1 - // Concrete split, not just the sum: all occurrences are in the definition - // file, so they are graph-confidence and text_search is zero. - expect(result.graph_edits).toBe(OCCURRENCE_LINES.length); - expect(result.text_search_edits).toBe(0); - - const edits = editsFor(result, 'src/lib.rs'); - expect(edits.map((e) => e.line).sort((a, b) => a - b)).toEqual(OCCURRENCE_LINES); - expect(edits.every((e) => e.confidence === 'graph')).toBe(true); - - // A dry run leaves the file untouched. - const onDisk = await fs.readFile(path.join(tmpDir, 'src', 'lib.rs'), 'utf-8'); - expect(onDisk).toContain('rename_target'); - }); - - it('reports exactly what it wrote (apply)', async () => { - const result = await callRename(stubbedBackend(), tmpDir, { dry_run: false }); - - expect(result.applied).toBe(true); - expect(result.total_edits).toBe(OCCURRENCE_LINES.length); - - const onDisk = await fs.readFile(path.join(tmpDir, 'src', 'lib.rs'), 'utf-8'); - const renamedCount = (onDisk.match(/\brenamed_fn\b/g) || []).length; - const stragglers = (onDisk.match(/\brename_target\b/g) || []).length; - expect(renamedCount).toBe(OCCURRENCE_LINES.length); // all 4 rewritten - expect(stragglers).toBe(0); - - // The reported edit count equals the number of replacements that landed. - const reportedEdits = result.changes.reduce((n, c) => n + c.edits.length, 0); - expect(reportedEdits).toBe(renamedCount); - }); - - it('enumerates all occurrences across graph-ref and text-search files, keeping confidence per file', async () => { - // A graph-referencing file (not the definition) with MULTIPLE occurrences — - // the exact "one edit per file then break" bug's other original trigger. - const CALLER = 'use crate::rename_target;\nfn a() { rename_target(1); rename_target(2); }\n'; - // A file discovered only by ripgrep — the text_search branch. - const NOTES = '// see rename_target for details\n'; - await fs.writeFile(path.join(tmpDir, 'src', 'caller.rs'), CALLER, 'utf-8'); - await fs.writeFile(path.join(tmpDir, 'src', 'notes.rs'), NOTES, 'utf-8'); - // rg reports the definition file (already graph — exercises never-downgrade) - // and the text-only file. - execFileSyncMock.mockReturnValue('src/lib.rs\nsrc/notes.rs\n'); - - const backend = stubbedBackend({ - ...EMPTY_INCOMING, - calls: [{ filePath: 'src/caller.rs' }], + it('previews bound occurrences, including callers absent from graph incoming files', async () => { + const result = await callRename(stubbedBackend(), tmpDir); + expect(result).toMatchObject({ + status: 'success', + applied: false, + result_version: 2, + total_edits: 4, + semantic_edits: 4, + graph_edits: 0, + text_search_edits: 0, + text_search: 'not_used', + files_affected: 2, }); - const result = await callRename(backend, tmpDir, { dry_run: true }); - - const callerOcc = occurrenceLines(CALLER).length; // 3 - const notesOcc = occurrenceLines(NOTES).length; // 1 - - expect(result.files_affected).toBe(3); - expect(result.total_edits).toBe(OCCURRENCE_LINES.length + callerOcc + notesOcc); - // Split is concrete: definition + graph-ref file are graph; the rg-only file - // is text_search. A file reached by both graph and rg keeps graph (never - // downgraded). - expect(result.graph_edits).toBe(OCCURRENCE_LINES.length + callerOcc); - expect(result.text_search_edits).toBe(notesOcc); - - expect(editsFor(result, 'src/lib.rs').every((e) => e.confidence === 'graph')).toBe(true); - expect(editsFor(result, 'src/caller.rs').map((e) => e.confidence)).toEqual(['graph', 'graph']); - expect(editsFor(result, 'src/notes.rs').map((e) => e.confidence)).toEqual(['text_search']); + const edits = result.changes.flatMap((change: any) => change.edits); + expect(edits.every((edit: any) => edit.confidence === 'semantic')).toBe(true); + expect(edits.every((edit: any) => edit.old_text === 'close')).toBe(true); + expect( + result.changes.find((change: any) => change.file_path === 'src/writer.ts').edits, + ).toHaveLength(3); + expect(execFileSyncMock).not.toHaveBeenCalled(); + expect(await fs.readFile(path.join(tmpDir, 'src/writer.ts'), 'utf8')).toBe(SOURCE); }); - it('reports only files that landed when a write fails mid-apply (#2605 partial)', async () => { - const CALLER = 'fn a() { rename_target(1); rename_target(2); }\n'; - await fs.writeFile(path.join(tmpDir, 'src', 'caller.rs'), CALLER, 'utf-8'); - const backend = stubbedBackend({ ...EMPTY_INCOMING, calls: [{ filePath: 'src/caller.rs' }] }); - - // caller.rs write throws; lib.rs succeeds. - vi.spyOn(fsPromises, 'writeFile').mockImplementation( - async (p: Parameters[0]) => { - if (String(p).endsWith(`${path.sep}caller.rs`)) { - throw Object.assign(new Error('EACCES'), { code: 'EACCES' }); - } - }, - ); - + it('applies only displayed spans, preserving unrelated same-line keys and symbols', async () => { + const backend = stubbedBackend(); + const preview = await callRename(backend, tmpDir); const result = await callRename(backend, tmpDir, { dry_run: false }); + expect(result).toMatchObject({ status: 'success', applied: true, total_edits: 4 }); + expect(result.changes).toEqual(preview.changes); + expect(await fs.readFile(path.join(tmpDir, 'src/writer.ts'), 'utf8')).toBe( + SOURCE.replace(' close() {}', ' closeWriter() {}').replaceAll( + 'writer.close()', + 'writer.closeWriter()', + ), + ); + expect(await fs.readFile(path.join(tmpDir, 'src/caller.ts'), 'utf8')).toBe( + CALLER.replace('writer.close()', 'writer.closeWriter()'), + ); + }); - expect(result.status).toBe('partial'); - expect(result.failed_files).toEqual(['src/caller.rs']); - // The failed file's edits are NOT reported as applied: totals describe only - // what reached disk (lib.rs), never the attempted caller.rs occurrences. - expect(result.files_affected).toBe(1); - expect(result.total_edits).toBe(OCCURRENCE_LINES.length); - expect(result.graph_edits).toBe(OCCURRENCE_LINES.length); - expect(result.changes.map((c) => c.file_path)).toEqual(['src/lib.rs']); + it('refuses unsupported languages without changing the target', async () => { + const rust = 'fn close() {}\nfn use_it() { close(); }\n'; + await fs.writeFile(path.join(tmpDir, 'src/lib.rs'), rust); + const backend = stubbedBackend({ + uid: 'Function:src/lib.rs:close', + kind: 'Function', + filePath: 'src/lib.rs', + startLine: 1, + endLine: 1, + }); + const result = await callRename(backend, tmpDir, { dry_run: false }); + expect(result).toMatchObject({ applied: false, planning_status: 'unsupported' }); + expect(await fs.readFile(path.join(tmpDir, 'src/lib.rs'), 'utf8')).toBe(rust); + }); + + it('rejects malformed dry_run instead of treating it as an apply request', async () => { + const result = await callRename(stubbedBackend(), tmpDir, { dry_run: 0 }); + expect(result.error).toMatch(/dry_run/); + expect(result.applied).toBe(false); + expect(await fs.readFile(path.join(tmpDir, 'src/writer.ts'), 'utf8')).toBe(SOURCE); + }); + + it('reports only the files and occurrences whose writes landed', async () => { + const originalWrite = fsPromises.writeFile.bind(fsPromises); + vi.spyOn(fsPromises, 'writeFile').mockImplementation(async (file, ...args: any[]) => { + if (String(file).endsWith(`${path.sep}writer.ts`)) throw new Error('EACCES'); + return (originalWrite as any)(file, ...args); + }); + const result = await callRename(stubbedBackend(), tmpDir, { dry_run: false }); + expect(result).toMatchObject({ + status: 'partial', + applied: true, + total_edits: 1, + semantic_edits: 1, + }); + expect(result.failed_files).toEqual(['src/writer.ts']); + expect(result.changes.map((change: any) => change.file_path)).toEqual(['src/caller.ts']); + expect(await fs.readFile(path.join(tmpDir, 'src/writer.ts'), 'utf8')).toBe(SOURCE); + expect(await fs.readFile(path.join(tmpDir, 'src/caller.ts'), 'utf8')).toContain( + 'writer.closeWriter()', + ); }); }); diff --git a/gitnexus/test/unit/rename-plan.test.ts b/gitnexus/test/unit/rename-plan.test.ts new file mode 100644 index 000000000..329206843 --- /dev/null +++ b/gitnexus/test/unit/rename-plan.test.ts @@ -0,0 +1,185 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; +import fs from 'node:fs/promises'; +import os from 'node:os'; +import path from 'node:path'; +import { executeRenamePlan, type RenamePlan } from '../../src/mcp/local/rename/rename-plan.js'; + +describe('exact rename plan application', () => { + let root: string; + beforeEach(async () => { + root = await fs.mkdtemp(path.join(os.tmpdir(), 'rename-plan-')); + }); + afterEach(async () => { + vi.restoreAllMocks(); + await fs.rm(root, { recursive: true, force: true }); + }); + + async function plan(files: Record): Promise { + for (const [name, text] of Object.entries(files)) + await fs.writeFile(path.join(root, name), text); + return { + symbol: { name: 'close', filePath: 'a.ts', startLine: 1 }, + new_name: 'finish', + coverage: { + name: 'typescript', + version: '5.9.3', + scope: 'project', + tsconfig_path: 'tsconfig.json', + source_file_count: Object.keys(files).length, + }, + snapshots: new Map( + Object.entries(files).map(([file, text]) => [path.join(root, file), text]), + ), + edits: Object.entries(files).flatMap(([file_path, text]) => { + const start = text.indexOf('close'); + return [{ file_path, start, length: 5, old_text: 'close', new_text: 'finish' }]; + }), + }; + } + + it('counts occurrences on one line and preserves unrelated keys, CRLF and UTF-16 offsets', async () => { + const text = '// 🐈\r\nconst hooks = { close: () => writer.close() }; writer.close();\r\n'; + const p = await plan({ 'a.ts': text }); + const starts = [text.indexOf('writer.close') + 7, text.lastIndexOf('writer.close') + 7]; + p.edits = starts.map((start) => ({ + file_path: 'a.ts', + start, + length: 5, + old_text: 'close', + new_text: 'complete', + })); + const preview = await executeRenamePlan(root, p, true); + expect(preview).toMatchObject({ + status: 'success', + applied: false, + application_status: 'not_requested', + total_edits: 2, + semantic_edits: 2, + }); + expect(preview.changes[0].edits.map((e) => e.line)).toEqual([2, 2]); + expect(await fs.readFile(path.join(root, 'a.ts'), 'utf8')).toBe(text); + const applied = await executeRenamePlan(root, p, false); + expect(applied).toMatchObject({ + status: 'success', + applied: true, + application_status: 'applied', + total_edits: 2, + }); + expect(await fs.readFile(path.join(root, 'a.ts'), 'utf8')).toBe( + '// 🐈\r\nconst hooks = { close: () => writer.complete() }; writer.complete();\r\n', + ); + }); + + it('deduplicates identical ranges while retaining full affix replacements', async () => { + const p = await plan({ 'a.ts': 'export { close };\nconst obj = { close };' }); + p.edits[0].new_text = 'finish as close'; + p.edits.push({ ...p.edits[0] }); + const start = p.snapshots.get(path.join(root, 'a.ts'))!.lastIndexOf('close'); + p.edits.push({ + file_path: 'a.ts', + start, + length: 5, + old_text: 'close', + new_text: 'close: finish', + }); + expect(await executeRenamePlan(root, p, false)).toMatchObject({ + total_edits: 2, + semantic_edits: 2, + }); + expect(await fs.readFile(path.join(root, 'a.ts'), 'utf8')).toBe( + 'export { finish as close };\nconst obj = { close: finish };', + ); + }); + + it.each(['overlap', 'unexpected text', 'stale snapshot'] as const)( + 'blocks all writes on %s', + async (failure) => { + const p = await plan({ 'a.ts': 'close()', 'b.ts': 'close()' }); + if (failure === 'overlap') + p.edits.push({ ...p.edits[0], start: 2, length: 3, old_text: 'ose' }); + if (failure === 'unexpected text') p.edits[1].old_text = 'wrong'; + if (failure === 'stale snapshot') await fs.writeFile(path.join(root, 'b.ts'), 'close(1)'); + const result = await executeRenamePlan(root, p, false); + expect(result).toMatchObject({ + status: 'error', + planning_status: 'blocked', + applied: false, + application_status: 'not_started', + total_edits: 0, + }); + expect(await fs.readFile(path.join(root, 'a.ts'), 'utf8')).toBe('close()'); + }, + ); + + it('checks snapshots of unedited project files too', async () => { + const p = await plan({ 'a.ts': 'close()', 'b.ts': 'close()' }); + p.edits.pop(); + await fs.writeFile(path.join(root, 'b.ts'), 'changed binding'); + expect(await executeRenamePlan(root, p, false)).toMatchObject({ + planning_status: 'blocked', + total_edits: 0, + }); + expect(await fs.readFile(path.join(root, 'a.ts'), 'utf8')).toBe('close()'); + }); + + it('rejects lexical traversal and symlink escapes', async () => { + const p = await plan({ 'a.ts': 'close()' }); + p.edits[0].file_path = '../outside.ts'; + expect(await executeRenamePlan(root, p, false)).toMatchObject({ planning_status: 'blocked' }); + const outside = await fs.mkdtemp(path.join(os.tmpdir(), 'rename-outside-')); + try { + await fs.writeFile(path.join(outside, 'b.ts'), 'close()'); + // Directory junctions exercise the same escape without Windows symlink privileges. + await fs.symlink( + outside, + path.join(root, 'link'), + process.platform === 'win32' ? 'junction' : 'dir', + ); + p.edits[0].file_path = 'link/b.ts'; + p.snapshots.set(path.join(root, 'link/b.ts'), 'close()'); + expect(await executeRenamePlan(root, p, false)).toMatchObject({ planning_status: 'blocked' }); + expect(await fs.readFile(path.join(outside, 'b.ts'), 'utf8')).toBe('close()'); + } finally { + await fs.rm(outside, { recursive: true, force: true }); + } + }); + + it.each([0, 1])('reports exactly landed edits when writing file %i fails', async (failAt) => { + const p = await plan({ 'a.ts': 'close()', 'b.ts': 'close()' }); + const original = fs.writeFile.bind(fs); + let calls = 0; + vi.spyOn(fs, 'writeFile').mockImplementation(async (...args) => { + if (calls++ === failAt) throw new Error('disk full'); + return original(...args); + }); + const result = await executeRenamePlan(root, p, false); + expect(result).toMatchObject({ + status: 'partial', + applied: failAt > 0, + application_status: failAt ? 'partial' : 'failed', + total_edits: failAt, + files_affected: failAt, + }); + expect(result.failed_files).toContain(failAt ? 'b.ts' : 'a.ts'); + expect(await fs.readFile(path.join(root, 'a.ts'), 'utf8')).toBe( + failAt ? 'finish()' : 'close()', + ); + expect(await fs.readFile(path.join(root, 'b.ts'), 'utf8')).toBe('close()'); + }); + + it('reports a race after an earlier write as partial', async () => { + const p = await plan({ 'a.ts': 'close()', 'b.ts': 'close()' }); + const original = fs.writeFile.bind(fs); + vi.spyOn(fs, 'writeFile').mockImplementation(async (...args) => { + await original(...args); + if (String(args[0]).endsWith('a.ts')) await original(path.join(root, 'b.ts'), 'user edit'); + }); + expect(await executeRenamePlan(root, p, false)).toMatchObject({ + status: 'partial', + applied: true, + total_edits: 1, + failed_files: ['b.ts'], + }); + expect(await fs.readFile(path.join(root, 'b.ts'), 'utf8')).toBe('user edit'); + }); +}); diff --git a/gitnexus/test/unit/shipped-skills-sync.test.ts b/gitnexus/test/unit/shipped-skills-sync.test.ts index 1df77026b..f61527067 100644 --- a/gitnexus/test/unit/shipped-skills-sync.test.ts +++ b/gitnexus/test/unit/shipped-skills-sync.test.ts @@ -311,10 +311,13 @@ describe('intended standard-skill improvements stay in every applicable copy', ( }, ); - it("uses the rename API's text_search vocabulary in every refactoring copy", () => { + it('teaches semantic project-scoped preview in every refactoring copy', () => { for (const file of standardSkillCopies('gitnexus-refactoring')) { const content = fs.readFileSync(file, 'utf-8'); - expect(content).toContain('text_search'); + expect(content).toContain('rename_preview'); + expect(content).toContain('tsconfig_path'); + expect(content).toContain('confidence: "semantic"'); + expect(content).not.toContain('graph (safe)'); expect(content).not.toContain('ast_search'); } }); diff --git a/gitnexus/test/unit/tools.test.ts b/gitnexus/test/unit/tools.test.ts index 7d6000039..aac1c1beb 100644 --- a/gitnexus/test/unit/tools.test.ts +++ b/gitnexus/test/unit/tools.test.ts @@ -2,7 +2,7 @@ * Unit Tests: MCP Tool Definitions * * Tests: GITNEXUS_TOOLS from tools.ts - * - All 19 tools are defined (per-repo + group_list/group_sync) + * - All 20 tools are defined (per-repo + group_list/group_sync) * - Each tool has valid name, description, inputSchema * - Required fields are correct * - Optional repo parameter is present on tools that need it @@ -23,8 +23,8 @@ const MUTATING_TOOLS = new Set(['rename', 'group_sync']); const OPEN_WORLD_READ_ONLY_TOOLS = new Set(['query']); describe('GITNEXUS_TOOLS', () => { - it('exports all tools (8 base + 1 explain + 1 pdg_query + 3 route/tool/shape + 1 api_impact + 1 trace + 2 group + read_file + grep)', () => { - expect(GITNEXUS_TOOLS).toHaveLength(19); + it('exports all tools (8 base + 1 explain + 1 pdg_query + 3 route/tool/shape + 1 api_impact + 1 trace + 2 group + read_file + grep + rename_preview)', () => { + expect(GITNEXUS_TOOLS).toHaveLength(20); }); it('contains all expected tool names', () => { @@ -38,6 +38,7 @@ describe('GITNEXUS_TOOLS', () => { 'detect_changes', 'check', 'rename', + 'rename_preview', 'impact', 'explain', 'pdg_query', @@ -357,7 +358,14 @@ describe('GITNEXUS_TOOLS', () => { }); it('per-repo tools have an optional branch scope param (#2106); group/list and checkout file tools do not', () => { - const noBranch = new Set(['list_repos', 'read_file', 'grep', ...GROUP_TOOLS]); + const noBranch = new Set([ + 'list_repos', + 'read_file', + 'grep', + 'rename', + 'rename_preview', + ...GROUP_TOOLS, + ]); for (const tool of GITNEXUS_TOOLS) { if (noBranch.has(tool.name)) { expect(tool.inputSchema.properties.branch, tool.name).toBeUndefined(); diff --git a/gitnexus/test/unit/typescript-rename.test.ts b/gitnexus/test/unit/typescript-rename.test.ts new file mode 100644 index 000000000..96b13a76f --- /dev/null +++ b/gitnexus/test/unit/typescript-rename.test.ts @@ -0,0 +1,369 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; +import fs from 'node:fs/promises'; +import os from 'node:os'; +import path from 'node:path'; +import ts from 'typescript-language-service'; +import { renameSymbol, type RenameSymbol } from '../../src/mcp/local/rename/rename-plan.js'; + +describe('configured TypeScript semantic rename', () => { + let root: string; + beforeEach(async () => { + root = await fs.mkdtemp(path.join(os.tmpdir(), 'ts-rename-')); + }); + afterEach(async () => { + vi.unstubAllEnvs(); + await fs.rm(root, { recursive: true, force: true }); + }); + + async function fixture(files: Record, config = 'tsconfig.json') { + await fs.writeFile( + path.join(root, config), + JSON.stringify({ + compilerOptions: { + target: 'ES2022', + module: 'CommonJS', + allowJs: true, + checkJs: true, + jsx: 'preserve', + skipLibCheck: true, + }, + include: ['**/*'], + }), + ); + for (const [file, text] of Object.entries(files)) { + await fs.mkdir(path.dirname(path.join(root, file)), { recursive: true }); + await fs.writeFile(path.join(root, file), text); + } + } + const method = (name: string, startLine: number, endLine = startLine): RenameSymbol => ({ + name, + kind: 'Method', + filePath: 'model.ts', + startLine, + endLine, + }); + + // Independent direct compiler oracle: compare every span and replacement, + // not just counts or text matches. It does not use the production planner. + function oracle(file: string, position: number, newName: string, config = 'tsconfig.json') { + const cfg = ts.readConfigFile(path.join(root, config), ts.sys.readFile); + const parsed = ts.parseJsonConfigFileContent(cfg.config, ts.sys, root); + const host: ts.LanguageServiceHost = { + ...ts.sys, + getCurrentDirectory: () => root, + getCompilationSettings: () => parsed.options, + useCaseSensitiveFileNames: () => ts.sys.useCaseSensitiveFileNames, + getScriptFileNames: () => parsed.fileNames, + getScriptVersion: () => '0', + getScriptSnapshot: (filename) => { + const text = ts.sys.readFile(filename); + return text === undefined ? undefined : ts.ScriptSnapshot.fromString(text); + }, + getDefaultLibFileName: (options) => ts.getDefaultLibFilePath(options), + }; + const service = ts.createLanguageService(host); + try { + return service + .findRenameLocations(path.join(root, file), position, false, false, { + providePrefixAndSuffixTextForRename: true, + })! + .map((location) => ({ + file_path: path.relative(root, location.fileName).split(path.sep).join('/'), + start: location.textSpan.start, + length: location.textSpan.length, + new_text: `${location.prefixText ?? ''}${newName}${location.suffixText ?? ''}`, + })) + .sort((a, b) => a.file_path.localeCompare(b.file_path) || a.start - b.start); + } finally { + service.dispose(); + } + } + function diagnostics(config = 'tsconfig.json') { + const cfg = ts.readConfigFile(path.join(root, config), ts.sys.readFile); + const parsed = ts.parseJsonConfigFileContent(cfg.config, ts.sys, root); + const program = ts.createProgram(parsed.fileNames, parsed.options); + return [...program.getSyntacticDiagnostics(), ...program.getSemanticDiagnostics()] + .map((d) => `${d.code}:${ts.flattenDiagnosticMessageText(d.messageText, ' ')}`) + .sort(); + } + async function assertOracle(symbol: RenameSymbol, newName: string, config?: string) { + const source = await fs.readFile(path.join(root, symbol.filePath), 'utf8'); + const offset = + source + .split('\n') + .slice(0, symbol.startLine - 1) + .join('\n').length + (symbol.startLine > 1 ? 1 : 0); + const position = source.indexOf(symbol.name, offset); + const expected = oracle(symbol.filePath, position, newName, config); + const baseline = diagnostics(config); + const preview = await renameSymbol(root, symbol, { new_name: newName, tsconfig_path: config }); + expect(preview.status, JSON.stringify(preview)).toBe('success'); + expect(preview.coverage).toMatchObject({ + name: 'typescript', + version: '5.9.3', + scope: 'project', + tsconfig_path: config ?? 'tsconfig.json', + }); + expect( + preview.changes.flatMap((file) => + file.edits.map((edit) => ({ + file_path: file.file_path, + start: edit.start, + length: edit.length, + new_text: edit.new_text, + })), + ), + ).toEqual(expected); + expect(preview).toMatchObject({ + graph_edits: 0, + text_search_edits: 0, + text_search: 'not_used', + semantic_edits: expected.length, + }); + const applied = await renameSymbol(root, symbol, { + new_name: newName, + tsconfig_path: config, + dry_run: false, + }); + expect(applied).toMatchObject({ + status: 'success', + applied: true, + total_edits: expected.length, + }); + expect(diagnostics(config)).toEqual(baseline); + return preview; + } + + it('finds map iteration callers while preserving an unrelated close method and prose without rg', async () => { + await fixture({ + 'model.ts': + 'export class SyncCsvWriter {\n close() {}\n}\nexport class PdgEmitSink { close() {} }\n', + 'caller.ts': + 'import { SyncCsvWriter, PdgEmitSink } from "./model";\nconst writers = new Map();\nfor (const w of writers.values()) { w.close(); w.close(); }\nnew PdgEmitSink().close(); // close\nconst title = "close";\n', + }); + vi.stubEnv('PATH', ''); + const result = await assertOracle(method('close', 2), 'finish'); + expect(result.total_edits).toBe(3); + expect(await fs.readFile(path.join(root, 'caller.ts'), 'utf8')).toContain( + 'new PdgEmitSink().close(); // close\nconst title = "close";', + ); + }); + + it('keeps a hook key beside this.resolve and follows base and derived overrides', async () => { + await fixture({ + 'model.ts': + 'export class ScopeTreeHarvester {\n resolve(n: number) { return n; }\n hooks = { resolve: (n: number) => this.resolve(n) };\n}\nexport class Derived extends ScopeTreeHarvester {\n override resolve(n: number) { return super.resolve(n); }\n}\nnew Derived().resolve(1); // resolve\n', + }); + await assertOracle(method('resolve', 2), 'lookup'); + const text = await fs.readFile(path.join(root, 'model.ts'), 'utf8'); + expect(text).toContain('hooks = { resolve: (n: number) => this.lookup(n) }'); + expect(text).toContain('override lookup'); + }); + + it('preserves compiler affixes for barrels, shorthand and destructuring', async () => { + await fixture({ + 'model.ts': + 'export const target = 1;\nconst obj = { target };\nconst { target: local } = obj;\n', + 'barrel.ts': 'export { target } from "./model";\n', + 'caller.ts': 'import { target } from "./barrel";\nconst result = target;\n', + }); + const result = await assertOracle( + { name: 'target', kind: 'Const', filePath: 'model.ts', startLine: 1, endLine: 1 }, + 'renamed', + ); + expect( + result.changes.flatMap((c) => c.edits).some((e) => e.new_text.includes(' as target')), + ).toBe(true); + expect(await fs.readFile(path.join(root, 'model.ts'), 'utf8')).toContain('{ target: renamed }'); + expect(await fs.readFile(path.join(root, 'caller.ts'), 'utf8')).toContain( + 'const result = target;', + ); + }); + + it('requires a config choice and allows an explicit nonstandard config', async () => { + await fixture({ + 'model.ts': 'export function target() {}', + 'test.ts': 'import { target } from "./model"; target();', + }); + await fs.writeFile( + path.join(root, 'tsconfig.test.json'), + JSON.stringify({ extends: './tsconfig.json' }), + ); + const symbol = { name: 'target', kind: 'Function', filePath: 'model.ts', startLine: 1 }; + const ambiguous = await renameSymbol(root, symbol, { new_name: 'changed', dry_run: false }); + expect(ambiguous).toMatchObject({ + planning_status: 'blocked', + applied: false, + code: 'ambiguous_project', + candidates: ['tsconfig.json', 'tsconfig.test.json'], + }); + await fs.rename(path.join(root, 'tsconfig.test.json'), path.join(root, 'custom-project.json')); + await assertOracle(symbol, 'changed', 'custom-project.json'); + }); + + it.each([ + ['main.js', 'jsconfig.json', '$target', 'δelta'], + ['main.jsx', 'jsconfig.json', 'δelta', '$target'], + ['main.tsx', 'tsconfig.json', '$target', 'δelta'], + ])('supports %s and scanner-validated identifiers', async (file, config, name, next) => { + await fixture({ [file]: `export function ${name}() { return 1; }\n${name}();` }, config); + await assertOracle({ name, kind: 'Function', filePath: file, startLine: 1 }, next, config); + }); + + it.each(['class', 'a-b', ' foo', 'foo ', 'close', 'foo/*x*/', '\\u0061'])( + 'rejects unsafe or unchanged new name %s', + async (new_name) => { + await fixture({ 'model.ts': 'class A {\nclose() {}\n}' }); + expect( + await renameSymbol(root, method('close', 2), { new_name, dry_run: false }), + ).toMatchObject({ planning_status: 'blocked', applied: false }); + expect(await fs.readFile(path.join(root, 'model.ts'), 'utf8')).toContain('close()'); + }, + ); + + it.each([ + 'missing config', + 'malformed config', + 'not member', + 'references', + 'unsupported language', + ] as const)('refuses %s without writes', async (reason) => { + await fixture({ 'model.ts': 'class A {\nclose() {}\n}' }); + let symbol = method('close', 2); + if (reason === 'missing config') await fs.unlink(path.join(root, 'tsconfig.json')); + if (reason === 'malformed config') + await fs.writeFile(path.join(root, 'tsconfig.json'), '{ "compilerOptions":'); + if (reason === 'not member') + await fs.writeFile(path.join(root, 'tsconfig.json'), '{ "files": [] }'); + if (reason === 'references') + await fs.writeFile( + path.join(root, 'tsconfig.json'), + '{ "references": [{ "path": "./other" }], "include": ["model.ts"] }', + ); + if (reason === 'unsupported language') symbol = { ...symbol, filePath: 'model.rs' }; + expect(await renameSymbol(root, symbol, { new_name: 'finish', dry_run: false })).toMatchObject({ + planning_status: 'unsupported', + applied: false, + total_edits: 0, + }); + expect(await fs.readFile(path.join(root, 'model.ts'), 'utf8')).toContain('close()'); + }); + + it.each([ + { name: 'close', kind: 'Method', filePath: 'model.ts', startLine: 2, endLine: 3 }, + { name: 'close', kind: 'Function', filePath: 'model.ts', startLine: 2, endLine: 2 }, + { name: 'close', kind: 'Method', filePath: 'model.ts', startLine: 1, endLine: 3 }, + { + uid: 'Method:model.ts:Other.close#0', + name: 'close', + kind: 'Method', + filePath: 'model.ts', + startLine: 2, + endLine: 2, + }, + ])('rejects stale or mismatched declaration identity: %j', async (symbol) => { + await fixture({ 'model.ts': 'class A {\nclose() {}\n}' }); + expect(await renameSymbol(root, symbol, { new_name: 'finish', dry_run: false })).toMatchObject({ + planning_status: 'blocked', + code: 'stale_symbol', + total_edits: 0, + }); + }); + + it('rejects ambiguous same-line declarations rather than choosing the first', async () => { + await fixture({ 'model.ts': 'class A { close() {} } class B { close() {} }' }); + expect(await renameSymbol(root, method('close', 1), { new_name: 'finish' })).toMatchObject({ + planning_status: 'blocked', + code: 'stale_symbol', + }); + }); + + it('uses the indexed owner to distinguish same-line method declarations', async () => { + await fixture({ + 'model.ts': + 'class A { close() {} } class B { close() {} }\nnew A().close(); new B().close();', + }); + const result = await renameSymbol( + root, + { ...method('close', 1), uid: 'Method:model.ts:A.close#0' }, + { new_name: 'finish', dry_run: false }, + ); + expect(result).toMatchObject({ status: 'success', total_edits: 2 }); + expect(await fs.readFile(path.join(root, 'model.ts'), 'utf8')).toBe( + 'class A { finish() {} } class B { close() {} }\nnew A().finish(); new B().close();', + ); + }); + + it('preserves a UTF-8 BOM and astral characters preceding compiler spans', async () => { + const source = '\ufeff// 🐈\r\nexport function target() {}\r\ntarget();\r\n'; + await fixture({ 'model.ts': source }); + const result = await renameSymbol( + root, + { name: 'target', kind: 'Function', filePath: 'model.ts', startLine: 2 }, + { new_name: 'changed', dry_run: false }, + ); + expect(result).toMatchObject({ status: 'success', total_edits: 2 }); + expect(await fs.readFile(path.join(root, 'model.ts'), 'utf8')).toBe( + '\ufeff// 🐈\r\nexport function changed() {}\r\nchanged();\r\n', + ); + }); + + it('refuses bytes that cannot round-trip through UTF-8', async () => { + await fixture({ 'model.ts': 'export function target() {}\n' }); + const original = Buffer.concat([ + Buffer.from('export function target() {}\n// '), + Buffer.from([0xff]), + ]); + await fs.writeFile(path.join(root, 'model.ts'), original); + expect( + await renameSymbol( + root, + { name: 'target', kind: 'Function', filePath: 'model.ts', startLine: 1 }, + { new_name: 'changed', dry_run: false }, + ), + ).toMatchObject({ + planning_status: 'unsupported', + code: 'unsupported_encoding', + applied: false, + }); + expect(await fs.readFile(path.join(root, 'model.ts'))).toEqual(original); + }); + + it('refuses unsupported computed declaration syntax', async () => { + await fixture({ 'model.ts': 'class A { ["close"]() {} }' }); + expect( + await renameSymbol(root, method('close', 1), { new_name: 'finish', dry_run: false }), + ).toMatchObject({ planning_status: 'blocked', code: 'stale_symbol', total_edits: 0 }); + expect(await fs.readFile(path.join(root, 'model.ts'), 'utf8')).toBe( + 'class A { ["close"]() {} }', + ); + }); + + it('rejects a semantic location outside the repository', async () => { + const outside = await fs.mkdtemp(path.join(os.tmpdir(), 'rename-dependency-')); + try { + const external = path.join(outside, 'outside.ts'); + await fs.writeFile( + external, + 'import { target } from ' + JSON.stringify(path.join(root, 'model')) + '; target();', + ); + await fixture({ 'model.ts': 'export function target() {}' }); + await fs.writeFile( + path.join(root, 'tsconfig.json'), + JSON.stringify({ files: ['model.ts', external] }), + ); + expect( + await renameSymbol( + root, + { name: 'target', kind: 'Function', filePath: 'model.ts', startLine: 1 }, + { new_name: 'changed', dry_run: false }, + ), + ).toMatchObject({ planning_status: 'blocked', applied: false }); + expect(await fs.readFile(path.join(root, 'model.ts'), 'utf8')).toBe( + 'export function target() {}', + ); + } finally { + await fs.rm(outside, { recursive: true, force: true }); + } + }); +});