Add LSP tool

[sdjidjev]

Official LSP Spec

Adds LSP tool functionality to Octo!
Starting out with these actions available for Octo to use, and planning to add more later:

- definition
  - location where a symbol was originally defined
- references
  - find all references to a symbol
- hover
  - type info and documentation for a symbol
- diagnostics
  - errors and warnings for a file
- implementation
  - similar to defintion, but jumps past interfaces and abstract classes to the code that implements them
- documentSymbol
  - list all symbols in a file
- incomingCalls
  - find callers of a symbol
- outgoingCalls
  - find callees of a symbol

During a LSP tool call, if a recommended LSP server is found to perform an LSP action for a file, but the user doesn't have it installed yet, we show a LspRecommendationPrompt to let Octo auto-install the LSP and continue operations:

(in unchained mode, the LSP will auto install using the shell command from the list of pre-approved LSP recommendations)

I commented by hand a lot of the LSP specific behavior since some of it is not general knowledge and there are some nuances.

To disable a specific LSP server recommendation, in ~/.config/octofriend/.octofriend.json5 you can do:

  lsp: {
    'typescript-language-server': {
      disabled: true,
    },
  }

or if you want all LSP recommendations disabled:

lsp: false,

Users can define their own LSP server configs in their project's directory in a file in .octofriend/lsp.json5
Example:

{
  'typescript-language-server': {
    serverName: 'typescript-language-server',
    command: [
      'typescript-language-server',
      '--stdio',
    ],
    extensions: [
      '.ts',
      '.tsx',
      '.js',
      '.jsx',
      '.mjs',
      '.cjs',
      '.mts',
      '.cts',
    ],
    rootCandidates: [
      'package.json',
      'tsconfig.json',
      'jsconfig.json',
    ],
  },
}

Screenshots of some LSP actions at work!

* - Octofriend should probably start having more detailed documentation on what LSP actions are supported and how to configure the setup locally (i'm inspired by how opencode does theirs), so will definitely be a follow-up task.

PS - I know this PR is super big, idk if it would be easier to review if it was chopped up into chunks but I think one of y'all mentioned you like each PR should include the whole feature so I'll leave it as-is. Since it was already so big, I left out some features (like initializationOptions, workspaceSymbols, LSPs that can't be installed with 1 shell command, etc.) but can add those as a follow-up!
That said, lmk if you need any clarification or PR separating!!

[reissbaker]

Approach generally looks good to me! Left some comments on the tool impl + a React component question.

One note: when I use Octo and it gets (for example) compiler output from tsc, I've noticed it often can't actually tell what line numbers map to what lines. For example, if it sees there's an error on line 101, it might not correctly figure out that means:

const x = Math.floor(); // haha no input, error

because we don't ever show it numbered lines; i.e. we'll send it an entire file, and it has to essentially internally count the lines itself.

I've noticed that Claude often internally compiles files down to lines with actual line numbers; for example, something like:

filename: README.md
1. # README.md
2. Hello, this is the
3. README for the
4. tool Octofriend

Have you noticed issues with Octo's ability to call different LSP tools accurately, since it's not great at guessing line/col numbers and some of these tools require specific line or column numbers?

I wonder if we should start compiling down file output to include line numbers in the compilers, so that Octo can use line-number-based tooling more effectively.

I also wonder about the character/column line numbers from the perspective of "can an LLM correctly use this most of the time": I would guess many LLMs are bad at counting characters in a line. If so, it might be worth exposing a different API for the column tooling, e.g. a string to search for in the line (and then on our end, use .indexOf(searchString) + 1 before passing to the LSP server).

[reissbaker]

You don't need to add this to the system prompt! A better place to add it is to the LSP tool itself, using Structural's .comment(...) function. Structural automatically converts .comment(...) calls into JSON Schema descriptions when using toJSONSchema(...), so that way the instructions will exist if the tool exists and will be skipped otherwise! LLMs might also respect the JSON Schema descriptions with tool defs more than just pure system prompts, although that will depend on the model.

[reissbaker]

IMO we should make it clear to an LLM what languages we have LSP servers configured for, so that they don't bother calling the tool if they're working in a language we don't have a definition for. You can do this by calling .comment(...) in the defineTool function, so that you have full access to the config file and can tell what LSP servers are options for the LLM. The LSP file types are probably just:

• The built-in ones, and
• Whatever is defined in the config.json5

[reissbaker]

Since this isn't used inside state.ts, should this be defined in a different file (i.e. where it's used)?

[reissbaker]

I think you can encode the types and requirements more elegantly, and in a way that's less likely to result in the LLM making mistakes and also leans on Structural for validation. It looks like you need different arguments for different subcommands; there are basically two ways to do this:

1. Encode this in each action, or
2. Make multiple tools for each action

(2) sounds wasteful, but — originally, file editing was done like (1), but I saw some open-source models were kinda too dumb to figure out complex schemas, and breaking it out into multiple tools worked better.

I think it's worth trying (1) for elegance, since it's closer to your current implementation anyway. But, it may be the case we eventually denormalize all of these actions into separate tools, if it helps the models be smarter. Anyway, here's how I'd do (1):

const LineSchema = t.num.comment(
  "1-indexed line number"
);
const CharSchema = t.num.comment(
  "1-indexed column number"
);
const DefinitionSchema = t.subtype({
  action: t.value("definition"),
  line: LineSchema,
  character: CharSchema,
});
const ReferencesSchema = t.subtype({
  action: t.value("references"),
  line: LineSchema,
  character: CharSchema,
});

// ...etc

const ArgumentSchema = t.subtype({
  filePath: t.str,
  action: DefinitionSchema.or(
    ReferencesSchema
  ).or(
    // ...
  ),
});

That way, there's a few advantages:

1. Structural can automatically guarantee the type is correct without runtime checks
2. The LLM can see the exact type it needs without needing to jump around mentally from comments (unclear whether this is valuable, but maybe?)
3. Inference engines that support "strict" function calling, where the LLM output is constrained based on a JSON schema passed in, will guarantee that the LLM never makes a mistake on this call. (Sadly we don't support strict calling on all models, but we do on some at least)

During this refactor, you may be tempted to extract out a shared definition like:

const RequiresAll = t.subtype({
  line: LineSchema,
  character: CharSchema,
});

const DefinitionSchema = RequiresAll.and(t.subtype({
  action: t.value("definition"),
}));

However — this actually won't work for many inference engines. Sadly many inference engines are written in Python by monkeys, and so they don't support exotic features such as JSON Schema allOf, which is what .and(...) compiles down to. Instead, if you want to do that, you'll need to denormalize it in TypeScript using splats:

const requiresAll = {
  line: LineSchema,
  character: CharSchema,
} as const;

const DefinitionSchema = t.subtype({
  action: t.value("definition"),
  ...requiresAll,
});

[sdjidjev]

@reissbaker - thanks for the comments! However ran into something trying to implement suggestion (1).

I definitely prefer the code elegance of option (1), but I ran into some issues where for example GLM4.7 was passing back a tool request where it was double-stringifying (because of the nested objects)
If I change octo's compiler to recursively decode stringified tool arguments it does work, but wondering if we should avoid that instead and do option (2) instead.

When I was first building this out, I thought about doing option (2) and having 1 tool per LSP action but was told by AI that Tool Saturation could maybe(?) be an issue and also I saw that OpenCode just has one LSP tool for all actions so decided to stick to how they were doing it.

However I think that having lots of actions within a LSP tool can't be that much different from having several LSP tools so if (2) sounds better to you I can do it that way instead! Or if you think it's better to recursively decode tool arguments so we can stick with option (1) I'm also down for that!

[reissbaker]

I might not be following the flow of the code correctly, but this feels a little flash-of-content-y to me, since lspRecommendation is set by an async function inside of a useEffect. Do we need a loading state, to render some interstitial "loading LSP..." text while we decide which thing to render?

[sdjidjev]

Ah I could see how it smells like such but it's the same flow as how whitelist is done in this function!
That is to say, there is no flash of content and the prompt only shows up if it's needed.