Phase 6: Add success messages to commands and fixture output expectation

[tonytvo]

Commands now return descriptive result messages (rename, extract-variable, sort-methods), and fixture .expected.out files capture their console output for regression testing. Also splits performRename to satisfy the 12-line function size limit.

[Copilot]

Pull request overview

Adds descriptive success messages to several refactoring commands and updates fixture-based regression expectations to capture console output.

Changes:

• Return RefactoringCommandResult(message) from rename, extract-variable, and sort-methods commands.
• Add/refresh .expected.out fixtures to assert stdout messages in regression tests.
• Refactor performRename into smaller helpers to satisfy the function size limit.

Reviewed changes

Copilot reviewed 27 out of 27 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
tests/fixtures/commands/sort-methods/simple-class.expected.out	Adds expected stdout message for sort-methods fixture
tests/fixtures/commands/sort-methods/recursive-methods.expected.out	Adds expected stdout message for sort-methods fixture
tests/fixtures/commands/rename/simple-variable.expected.out	Adds expected stdout message for rename fixture
tests/fixtures/commands/rename/scope-isolation.expected.out	Adds expected stdout message for rename fixture
tests/fixtures/commands/rename/nested-scopes.expected.out	Adds expected stdout message for rename fixture
tests/fixtures/commands/rename/location-simple-variable.expected.out	Adds expected stdout message for rename fixture
tests/fixtures/commands/rename/function-argument.expected.out	Adds expected stdout message for rename fixture
tests/fixtures/commands/rename/block-scope.expected.out	Adds expected stdout message for rename fixture
tests/fixtures/commands/rename/arrow-function.expected.out	Adds expected stdout message for rename fixture
tests/fixtures/commands/extract-variable/simple-expression.expected.out	Adds expected stdout message for extract-variable fixture
tests/fixtures/commands/extract-variable/scope-isolation.expected.out	Adds expected stdout message for extract-variable fixture
tests/fixtures/commands/extract-variable/nested-calls.expected.out	Adds expected stdout message for extract-variable fixture
tests/fixtures/commands/extract-variable/name-conflict.expected.out	Adds expected stdout message for extract-variable fixture
tests/fixtures/commands/extract-variable/multiple-occurrences.expected.out	Adds expected stdout message for extract-variable fixture
tests/fixtures/commands/extract-variable/multi-arg-method-reference.expected.out	Adds expected stdout message for extract-variable fixture
tests/fixtures/commands/extract-variable/multi-arg-method-call.expected.out	Adds expected stdout message for extract-variable fixture
tests/fixtures/commands/extract-variable/location-simple-expression.expected.out	Adds expected stdout message for extract-variable fixture
tests/fixtures/commands/extract-variable/extract-variable-holding-function.expected.out	Adds expected stdout message for extract-variable fixture
tests/fixtures/commands/extract-variable/conditional-block.expected.out	Adds expected stdout message for extract-variable fixture
tests/fixtures/commands/extract-variable/complex-expression.expected.out	Adds expected stdout message for extract-variable fixture
tests/fixtures/commands/extract-variable/complex-expression-last-single-occurence.expected.out	Adds expected stdout message for extract-variable fixture
tests/fixtures/commands/extract-variable/complex-expression-last-selected.expected.out	Adds expected stdout message for extract-variable fixture
tests/fixtures/commands/extract-variable/complex-expression-first-single-occurence.expected.out	Adds expected stdout message for extract-variable fixture
tests/fixtures/commands/extract-variable/cli-location-format.expected.out	Adds expected stdout message for extract-variable fixture
src/core/commands/sort-methods-command.ts	Returns a success message including class name and method count
src/core/commands/rename-command.ts	Returns a success message including old/new names and rename count
src/core/commands/extract-variable-command.ts	Returns a success message including extracted name and replacement count (when --all)

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

The command always returns a "Successfully sorted …" message, even when no sorting is performed (e.g., classes with 0–1 methods, where shouldSkipSorting() prevents any reordering). This yields misleading outputs like "Successfully sorted 0/1 methods". Consider returning a flag/result from performMethodSorting indicating whether sorting actually happened (or the pre-sort method count) and emit a more accurate message such as "No sorting needed" when skipped.

[tonytvo]

Questions

• does it make sense to setup obserability on how the product is being used, errors, etc...?
• why there's no contributing guidelines?
  • is that because the quality check would cover most or all of the guildelines?
• how can I understand the flow of the code better?
  • after starting with the tests, the implementation finished pretty quickly, but I don't quite understand how the code works. Looking for some quick easy code flow to understand the code better
• how to use SAMPLE_USER_CLAUDE.md under dev-env-setup?
  • copy the file to ~/.claude/CLAUDE.md to use as personal Claude flavor
• feed the PR to my own AI review process below with teh following 2 files
  • CODE_REVIEW_TEMPLATE.md
  • code-review-patterns-practices.md

[tonytvo]

Branch/PR Information

• PR: #83
• Head branch: tonytvo:main
• Base branch: devill:main
• Author: Tony Trung Thanh Vo (tonytvo)
• Reviewer: Tony Vo
• Date: 2026-04-25
• CI Status: ✅ Passing (Node 20.x, 22.x)

---

Summary

Adds descriptive success messages to the rename, extract-variable, and sort-methods commands, returning them via RefactoringCommandResult(message). Backs these up with .expected.out fixture files for regression-testing console output across all 27 affected test cases. Also splits performRename into applyRename to satisfy the 12-line function size limit.

Files Changed

• src/core/commands/rename-command.ts — returns RefactoringCommandResult with old→new name and occurrence count
• src/core/commands/extract-variable-command.ts — returns message with variable name and optional occurrence count
• src/core/commands/sort-methods-command.ts — returns message with class name and method count
• tests/fixtures/commands/rename/*.expected.out (7 files) — new stdout fixture expectations
• tests/fixtures/commands/extract-variable/*.expected.out (16 files) — new stdout fixture expectations
• tests/fixtures/commands/sort-methods/*.expected.out (2 files) — new stdout fixture expectations

---

✅ Strengths

1. Comprehensive fixture coverage: Adding .expected.out for every existing fixture is thorough — no silent regressions possible on the new output path.

2. Occurrence counts in messages: Including how many sites were renamed/extracted is genuinely useful for AI agents — they can verify the operation touched the expected number of occurrences.

3. Consistent pluralization: occurrence${count === 1 ? '' : 's'} is correct and applied uniformly across all three commands.

4. Clean performRename split: applyRename is a focused, well-named helper that makes the rename flow easier to follow, not just a mechanical line-count fix.

---

⚠️ Issues & Recommendations

High Priority 🟠

1. Misleading success message when sort-methods is a no-op
   • Location: src/core/commands/sort-methods-command.ts:26-30
   • Description: performMethodSorting always returns { className, methodCount } regardless of whether shouldSkipSorting returned true. When a class has 0 or 1 methods, the output is "Successfully sorted 0 methods in class 'X'" — nothing was actually done.
   • Impact: Erodes trust in tool output (see "Misleading Success Messages" anti-pattern — a no-op reported as success is actively misleading to the AI agent consuming it). Copilot also flagged this.
   • Recommendation: Track whether sorting happened and emit a distinct message for the no-op case.
   • Suggested Change for performMethodSorting:

     private async performMethodSorting(targetClass: ClassDeclaration): Promise<{ className: string; methodCount: number; sorted: boolean }> {
       const methods = this.methodFinder.findMethods(targetClass);
       const className = targetClass.getName() ?? 'unknown';
       if (!this.shouldSkipSorting(methods)) {
         this.reorderMethodsInClass(targetClass, this.getSortedMethods(methods));
         return { className, methodCount: targetClass.getMethods().length, sorted: true };
       }
       return { className, methodCount: targetClass.getMethods().length, sorted: false };
     }

     And in execute, change the message construction to:

     const message = sorted
       ? `Successfully sorted ${methodCount} method${methodCount === 1 ? '' : 's'} in class '${className}'`
       : `Methods in class '${className}' are already in order`;

Medium Priority 🟡

2. transform() is now dead code for the rename path

   • Location: src/core/transformations/rename-variable-transformation.ts + src/core/commands/rename-command.ts:applyRename
   • Description: applyRename calls transformWithResult() directly. transform() internally also calls transformWithResult() and throws on failure — so transform() is now unreachable from this command's execution path. Two nearly-identical error-handling paths exist for the same operation.
   • Recommendation: Consider whether transform() is still needed by other callers, or whether the interface can be simplified to just transformWithResult(). Not blocking, but worth a follow-up.

3. rename: no-op rename (0 occurrences) succeeds silently

   • Location: src/core/commands/rename-command.ts:applyRename
   • Description: If transformResult.changesCount === 0, the message reads "Successfully renamed 'x' to 'y' (0 occurrences renamed)" — misleading for the same reason as sort-methods.
   • Recommendation: Guard with a check on count === 0 and throw or emit a distinct message.

Low Priority ⚪

4. Magic number 80 in rename-variable-transformation.ts
   • Location: src/core/transformations/rename-variable-transformation.ts:findIdentifierInNode
   • Description: node.getKind() === 80 — 80 is SyntaxKind.Identifier. Pre-existing, not introduced by this PR.
   • Recommendation: Replace with SyntaxKind.Identifier for clarity.

---

🔍 Questions to Consider

1. Occurrence count consistency: rename always shows (N occurrences renamed) even for N=1. extract-variable single path omits the count entirely. Should the AI agent always see a count, or only when --all is meaningful? Consistency either way helps.

2. Shared message builder: As more commands get messages, a small shared template convention could prevent structural drift across commands.

---

🧪 Testing Notes

Test Coverage

• Fixture tests added for all renamed/extracted/sorted scenarios
• CI passing on Node 20.x and 22.x
• No-op sort-methods case not covered by a fixture (class with 0 or 1 methods) — the misleading message wouldn't be caught without it

Test Gaps

• Add a fixture for sort-methods on a single-method or empty class to lock in the no-op message behavior once fixed.

---

✅ Overall Assessment

Decision: APPROVED WITH CHANGES

Summary: Solid, well-scoped PR. The fixture coverage is comprehensive and the occurrence counts in messages are a meaningful UX improvement for AI agents. The one thing to address before merge is the misleading "Successfully sorted 0 methods" output when sort-methods is a no-op — it directly contradicts the "Misleading Success Messages" anti-pattern and would confuse downstream agents. Fix that and self-merge.

Merge Checklist

• CI passing (Node 20.x, 22.x)
• No merge conflicts
• Fixture regression tests updated
• sort-methods no-op message fixed
• sort-methods no-op fixture added

[devill]

@tonytvo Thanks for the PR! Copilot made a valid observation about the sort commands message being potentially misleading in some edge cases, but to me that isn't a really important edge case.