[eli0shin]

Summary

Adds a new statusline command that outputs active LSP server names for integration with Claude Code's statusline display.

The command returns server names as a deduplicated, sorted, comma-separated string, making it easy to surface active LSP information in status displays.

Changes

• New statusline command: Returns comma-separated list of active LSP server IDs
• Daemon handler: Implements statusline request that deduplicates and sorts running server IDs
• Comprehensive tests: Validates output format, sorting, deduplication, and exit codes
• Changeset: Documents minor version bump for new feature

Technical Details

The implementation:

1. Queries the daemon for running servers via getRunningServers()
2. Extracts and deduplicates server IDs using Set
3. Sorts alphabetically for consistent output
4. Returns as comma-separated string
5. Exits silently if no daemon is running (no error output)

Output format is optimized for statusline consumption with minimal whitespace and predictable ordering.

Testing

Test coverage includes:

• Verification of sorted, comma-separated output format
• Validation that expected servers (typescript, json) are present
• Output format checks for trailing whitespace
• Exit code verification

[claude]

Pull Request Review: Add statusline command

Overall Assessment

This is a well-implemented feature that follows the codebase patterns and includes comprehensive tests. The code is clean and the functionality is straightforward. I have identified a few issues and suggestions for improvement.

---

Critical Issues

1. Inconsistent command implementation pattern

Location: src/commands/statusline.ts:5-18

Issue: The statusline command doesn't follow the established pattern used by other commands (status, logs, stop). It directly calls sendToExistingDaemon() instead of using runCommand().

Current implementation:

export function registerStatuslineCommand(program: Command) {
  program
    .command('statusline')
    .description('Show active LSP server names for statusline display')
    .action(async () => {
      try {
        const result = await sendToExistingDaemon('statusline', []);
        if (typeof result === 'string' && result.length > 0) {
          process.stdout.write(result + '\n');
        }
      } catch {
        // No daemon running — exit silently
      }
    });
}

Expected pattern (see src/commands/status.ts, src/commands/logs.ts):

export function registerStatuslineCommand(program: Command) {
  program
    .command('statusline')
    .description('Show active LSP server names for statusline display')
    .action(async (_options, command) => {
      const opts = command.optsWithGlobals();
      assertConfigFileOptions(opts);
      await runCommand('statusline', [], opts.configFile);
    });
}

Why this matters:

• The current implementation bypasses the runCommand() infrastructure, which handles daemon startup, error handling, and config file options
• This creates an inconsistency where statusline won't respect the --config-file flag that other commands support
• The error handling is less robust (generic catch vs. structured error handling in runCommand())
• It breaks the architectural pattern where commands go through runCommand() → ensureDaemonRunning() → sendToExistingDaemon()

---

Code Quality Issues

2. Silent failure may hide real errors

Location: src/commands/statusline.ts:14-15

Issue: The empty catch block silently swallows all errors, not just "daemon not running" errors. This could hide legitimate issues like network errors, permission problems, or bugs.

Current:

} catch {
  // No daemon running — exit silently
}

Suggestion: If you intend to keep the direct sendToExistingDaemon() call (not recommended - see issue #1), at least differentiate between expected and unexpected errors:

} catch (error) {
  // Only exit silently if daemon isn't running
  // Other errors should be reported
  if (error instanceof Error && error.message.includes('ENOENT')) {
    // Daemon not running - exit silently as intended
    return;
  }
  // Unexpected error - report it
  process.stderr.write(`Error: ${error instanceof Error ? error.message : 'Unknown error'}\n`);
  process.exit(1);
}

However, if you switch to using runCommand() (recommended), this becomes moot as runCommand() handles errors appropriately.

---

3. Missing explicit exit code handling

Location: src/commands/statusline.ts

Issue: The command doesn't explicitly handle the exit code, relying on implicit behavior. The tests verify exit code 0, but the code doesn't guarantee this in all paths.

Context: Looking at src/client.ts:434-500, most commands explicitly call process.exit() with specific codes. The statusline command should do the same for clarity and consistency.

If keeping current implementation:

try {
  const result = await sendToExistingDaemon('statusline', []);
  if (typeof result === 'string' && result.length > 0) {
    process.stdout.write(result + '\n');
  }
  process.exit(0); // Explicit success
} catch (error) {
  // ... error handling
}

---

Best Practices & Minor Issues

4. Test relies on timing assumptions

Location: tests/statusline.test.ts:11

Issue: The test includes a 2-second sleep to "let servers initialize," which makes tests slower and fragile.

// Give servers a moment to initialize
await new Promise((resolve) => setTimeout(resolve, 2000));

Suggestion: Consider polling the status until servers are ready, or accept that the test validates whatever state the daemon is in. Since the servers should be initialized from a previous test run in the suite, this sleep may not even be necessary.

// Alternative: Poll until ready or timeout
const maxWait = 5000;
const pollInterval = 100;
let elapsed = 0;
let result;
while (elapsed < maxWait) {
  result = await runCommandWithArgs(['statusline']);
  if (result.stdout.trim().length > 0) break;
  await new Promise((resolve) => setTimeout(resolve, pollInterval));
  elapsed += pollInterval;
}

---

5. Test makes assumptions about repository content

Location: tests/statusline.test.ts:17-24

Issue: The test assumes the repository will always have TypeScript and JSON files, which makes the test brittle.

// This project has .ts, .json, .sh, .yaml files so these servers should be active
expect(servers.includes('typescript')).toBe(true);
expect(servers.includes('json')).toBe(true);

Suggestion: Make the test more flexible:

// Verify we got some servers (non-empty output)
expect(servers.length).toBeGreaterThan(0);
// Verify format is correct (comma-separated, sorted)
const sorted = [...servers].sort();
expect(servers.join(', ')).toBe(sorted.join(', '));

Or keep the specific checks but add a comment explaining the dependency:

// This test depends on the repository having .ts and .json files
// If the repo structure changes significantly, these assertions may need updating
expect(servers.includes('typescript')).toBe(true);
expect(servers.includes('json')).toBe(true);

---

6. Inconsistent output formatting

Location: src/daemon.ts:206-210

Issue: The statusline command returns a bare string (no newline), but src/commands/statusline.ts:12 adds a newline. This is inconsistent with how other commands work.

Context: Looking at the logs command:

• src/daemon.ts:251-252: Returns LOG_PATH (no newline)
• src/client.ts:484: Adds newline via process.stdout.write(\${result}\n`)`

The statusline command follows this pattern, which is fine, but the daemon return value and command output handling could be better documented.

Suggestion: Add a comment in the daemon handler explaining the output format:

case 'statusline': {
  const runningServers = getRunningServers();
  const serverIDs = [...new Set(runningServers.map(s => s.serverID))].sort();
  // Return comma-separated list (newline added by command handler)
  return serverIDs.join(', ');
}

---

Positive Observations

✅ Good test coverage - Tests validate output format, sorting, deduplication, exit codes, and edge cases
✅ Clear documentation - Changeset and PR description explain the feature well
✅ Minimal implementation - The daemon handler is concise and efficient
✅ Follows existing patterns - Uses getRunningServers() and Set for deduplication like the status command
✅ Appropriate use of silent exit - Makes sense for statusline integration to not output errors

---

Recommendations

Priority 1 (Should Fix):

1. ✅ Refactor to use runCommand() pattern - Critical for consistency and maintainability
2. ⚠️ Improve error handling - Don't silently swallow unexpected errors

Priority 2 (Consider):

3. 📝 Remove or improve timing-dependent test - Makes tests faster and more reliable
4. 📝 Make tests less brittle - Don't assume specific servers will always be present

Priority 3 (Nice to Have):

5. 💡 Add documentation comment - Explain output format in daemon handler
6. 💡 Explicit exit codes - Match the pattern in other commands

---

Security & Performance

✅ No security concerns - Read-only operation, no user input processed
✅ No performance concerns - Minimal overhead, simple array operations
✅ No memory leaks - No resources held, quick execution

---

Conclusion

This is a solid implementation of a useful feature. The main concern is the architectural inconsistency with other commands. Once the command implementation is refactored to use runCommand(), this will be ready to merge.

Suggested Actions:

1. Refactor src/commands/statusline.ts to match the pattern in src/commands/status.ts and other commands
2. Consider addressing the test fragility issues
3. Add explicit exit code handling if desired

Let me know if you'd like me to provide a complete refactored version of the statusline command!

[claude]

Code Review: Add statusline command for active LSP server display

Summary

This PR adds a new statusline command that outputs active LSP server names as a sorted, comma-separated list for integration with Claude Code's statusline display. The implementation is clean and well-tested.

✅ Strengths

1. Clean Implementation: The daemon handler is concise and uses appropriate JavaScript patterns (Set for deduplication, Array methods for transformation)
2. Good Error Handling: The statusline command properly handles the case when no daemon is running by failing silently (no stderr output), which is appropriate for statusline usage
3. Consistent with Codebase: Follows existing patterns for command registration and daemon handlers
4. Well-Documented: PR description clearly explains the feature, implementation details, and testing approach
5. Proper Versioning: Includes changeset for minor version bump, appropriate for a new feature

🔍 Code Quality Observations

src/client.ts (lines 454-468)

• Error handling correctly distinguishes between connection errors (silent) and unexpected errors (stderr)
• The condition !('code' in error) is used to filter out connection errors - this works but could be more explicit
• Always exits with code 0, which is correct for statusline integration

src/daemon.ts (lines 206-210)

• Simple and efficient implementation using Set for deduplication
• Alphabetical sorting ensures deterministic output
• Returns empty string when no servers are running (handled gracefully by client)

src/commands/statusline.ts

• ⚠️ Missing configFile parameter: Unlike other commands (e.g., logs.ts), this command doesn't pass the configFile option to runCommand
• The signature is runCommand('statusline', []) but should be runCommand('statusline', [], opts.configFile) for consistency

🐛 Potential Issues

1. Missing configFile parameter (Minor bug)

// Current implementation
export function registerStatuslineCommand(program: Command) {
  program
    .command('statusline')
    .description('Show active LSP server names for statusline display')
    .action(async () => {
      await runCommand('statusline', []);
    });
}

Expected pattern (see src/commands/logs.ts for reference):

export function registerStatuslineCommand(program: Command) {
  program
    .command('statusline')
    .description('Show active LSP server names for statusline display')
    .action(async (_options, command) => {
      const opts = command.optsWithGlobals();
      assertConfigFileOptions(opts);
      await runCommand('statusline', [], opts.configFile);
    });
}

This matters because runCommand uses the configFile to determine the correct daemon socket path via SOCKET_PATH, which is computed based on the config file location.

🧪 Test Coverage

tests/statusline.test.ts

• ✅ Tests the happy path: verifies sorted, deduplicated output
• ✅ Uses proper test utilities (stripAnsi, runCommandWithArgs)
• ✅ Validates exit code
• ✅ Follows CLAUDE.md testing conventions (toBe with exact match, stripAnsi)
• ⚠️ Missing edge case tests:
  • What happens when no daemon is running? (Should exit 0 with empty output)
  • What happens when daemon has no servers? (Should exit 0 with empty output)
  • Test with multiple servers of the same type to verify deduplication

Suggested additional test cases:

test('should return empty output when no daemon is running', async () => {
  // Stop daemon first
  await runCommandWithArgs(['stop']);
  const result = await runCommandWithArgs(['statusline']);
  expect(result.exitCode).toBe(0);
  expect(stripAnsi(result.stdout)).toBe('');
});

test('should deduplicate servers with same ID in different roots', async () => {
  // This would require multiple projects with same server type
  // The current test implicitly verifies this but could be explicit
});

🔒 Security & Performance

• ✅ No security concerns identified
• ✅ Performance is good - O(n log n) due to sort, appropriate for small server lists
• ✅ No resource leaks or async issues

📋 Recommendations

1. Fix the configFile parameter in src/commands/statusline.ts to match other commands' patterns
2. Add edge case tests for no-daemon and no-servers scenarios
3. Consider adding a --help example showing expected output format

Overall Assessment

This is a solid, well-implemented feature with good documentation. The main issue is the missing configFile parameter which should be fixed before merge to maintain consistency with the rest of the codebase and ensure correct behavior when using custom config files.

Recommendation: Approve with minor changes requested

[claude]

Code Review: PR #52 - Add statusline command

Overall, this is a well-implemented feature with clean code and good test coverage. The implementation follows project conventions and integrates smoothly with the existing codebase. Below are my detailed findings:

---

✅ Strengths

1. Clean, focused implementation: The feature does exactly what it needs to without over-engineering
2. Follows existing patterns: Correctly mirrors the structure of other commands (stop-all, list) that don't require daemon startup
3. Good separation of concerns: Command registration, client handling, and daemon logic are properly separated
4. Appropriate error handling: Silently exits when daemon isn't running (correct for statusline use case), errors go to stderr
5. Proper output hygiene: Ensures stdout contains ONLY the server list, critical for statusline consumption
6. Changeset included: Properly documents the minor version bump

---

🔍 Code Quality Analysis

src/commands/statusline.ts ✅

• Follows the exact pattern of other commands
• Properly uses type guards with assertConfigFileOptions
• No issues found

src/daemon.ts (lines 206-210) ✅

• Clean implementation using Set for deduplication
• Alphabetical sorting ensures consistent output
• Concise and readable
• Note: Returns empty string when no servers running (verified by checking client.ts:457-459 which handles this correctly)

src/client.ts (lines 453-468) ✅

• Correctly uses sendToExistingDaemon instead of ensureDaemonRunning - important for statusline semantics
• Proper stdout/stderr separation
• Silent on connection errors (no daemon) - correct behavior
• The condition !('code' in error) at line 463 is interesting - this filters out connection errors (which have a 'code' property) from unexpected errors

Minor observation: The error filtering logic at line 463 could benefit from a brief comment explaining why 'code' property is checked, but it's not critical.

src/cli.ts (lines 17, 81) ✅

• Proper import and registration
• Follows alphabetical ordering convention

---

🧪 Test Coverage

tests/statusline.test.ts

Strengths:

• Tests the primary use case well
• Verifies sorting and deduplication
• Uses stripAnsi() correctly per CLAUDE.md guidelines
• Exit code verification included

Potential gaps:

1. No test for empty server list: What happens when no servers are running? The test relies on the status command having active servers. Consider adding:

   test('should return empty output when no servers running', async () => {
     await runCommandWithArgs(['stop-all']);
     const result = await runCommandWithArgs(['statusline']);
     expect(result.exitCode).toBe(0);
     expect(stripAnsi(result.stdout)).toBe('');
   });

2. No test for daemon not running: While the implementation handles this, a test would document the behavior:

   test('should exit cleanly when daemon not running', async () => {
     await runCommandWithArgs(['stop']);
     const result = await runCommandWithArgs(['statusline']);
     expect(result.exitCode).toBe(0);
     expect(stripAnsi(result.stdout)).toBe('');
     expect(stripAnsi(result.stderr)).toBe('');
   });

3. No test for multiple instances of same server: While deduplication is tested implicitly, an explicit test would be valuable.

---

🔒 Security Considerations

• ✅ No user input processing
• ✅ No file system operations
• ✅ No command injection vectors
• ✅ Read-only operation on daemon state

---

⚡ Performance Considerations

• ✅ Efficient Set-based deduplication (O(n) complexity)
• ✅ Native array sort (optimized in JavaScript engines)
• ✅ No unnecessary iterations or allocations
• ✅ Minimal string concatenation with join()

---

📝 Best Practices Alignment

Checked against CLAUDE.md:

• ✅ Uses TypeScript with strict mode
• ✅ ES modules syntax
• ✅ Follows existing command patterns
• ✅ Test uses exact string matching with toBe() (not toContain())
• ✅ Test uses stripAnsi() before assertions
• ✅ Changeset properly formatted

---

🐛 Potential Issues

None found - The implementation is solid.

One minor consideration: The output format uses ', ' (comma-space separator). This is fine for statusline display, but worth confirming this format meets Claude Code's statusline requirements. The consistent sorting ensures deterministic output regardless of server start order, which is good for UI stability.

---

📊 Summary

Category	Rating	Notes
Code Quality	⭐⭐⭐⭐⭐	Clean, idiomatic, follows patterns
Test Coverage	⭐⭐⭐⭐☆	Good coverage, could add edge cases
Security	⭐⭐⭐⭐⭐	No concerns
Performance	⭐⭐⭐⭐⭐	Optimal implementation
Documentation	⭐⭐⭐⭐⭐	Clear description, changeset included

---

🎯 Recommendation

APPROVE - This PR is ready to merge. The suggested test additions are nice-to-haves, not blockers. The implementation is clean, correct, and follows all project conventions.

The only actionable item would be adding tests for the edge cases mentioned above, but the current implementation handles them correctly, so this is optional.

Great work! 🚀