kimandClaude Sonnet 5
2a35f40c73
Builds out locode's Claude Code plugin parity: MCP servers, slash commands, sub-agents, hooks (12 lifecycle events), and skills, all loadable from a local path or git URL. Also adds image support (read_file, /import), @-mention file autocomplete, a /dashboard stats view, /export with an editable filename prompt, an expanded git tool (reset/stash/merge/rebase/delete_branch), and a redesigned status bar. On top of that, a review pass found and fixed 10 correctness/stability bugs: argument-injection in git reset/merge/rebase (a ref like "--hard" was parsed as a flag), tool-call image results interleaving with native tool_call_id messages and breaking OpenAI-compatible message ordering, backgrounded bash jobs still being silently killed by their original timeout with the kill masked as a clean exit, a SubagentStart hook's block being ignored, the sub-agent timeout clock starting before the hook it should exclude, a template-expansion bug that could re-substitute $1..$9 placeholders, unbounded background-job output buffers, a missing directory-target fallback in /export, and image size caps checked after reading the whole file instead of before. Session saves and exports now go through a shared atomic write-then-rename helper so a crash can't leave a truncated file. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
145 lines
9.7 KiB
Markdown
145 lines
9.7 KiB
Markdown
# Plan: Resolve locode's 9 known issues
|
||
|
||
## Goals
|
||
Fix all nine documented issues/limitations in one coherent pass, keep `typecheck` green, and add enough tests so `npm test` passes.
|
||
|
||
## Issues and implementation details
|
||
|
||
### 1. Grep bug: patterns starting with `-` are parsed as ripgrep flags
|
||
**File:** `src/tools/grep.ts`
|
||
**Approach:** Pass the pattern with ripgrep's `-e` option instead of as a positional argument, and put `--` before the path argument. This prevents any pattern (including `-foo`, `--foo`, `+foo`) from being interpreted as a flag.
|
||
**Test:** Add `src/tools/grep.test.ts` that mocks `execa` and verifies the generated args for a leading-dash pattern.
|
||
|
||
### 2. Security: session id is not sanitized before building the file path
|
||
**File:** `src/persistence/sessionStore.ts`
|
||
**Approach:**
|
||
- Derive a safe filename from the user-supplied id by replacing path separators and other unsafe characters with `_`.
|
||
- Keep the original id inside the record (it already is), but use the sanitized id for the filesystem lookup.
|
||
- Add a `safeId` helper and use it in `filePath`, `loadSession`, `deleteSession`, and `listSessions` so `--resume ../../etc/passwd` cannot escape the sessions directory.
|
||
**Test:** Add `src/persistence/sessionStore.test.ts` verifying that malicious ids are contained, normal ids still work, and `listSessions` ignores non-`.json` files.
|
||
|
||
### 3. Tests: `npm test` fails because there are no test files
|
||
**Approach:** Create the first test suite covering the fixes above. Add `vitest.config.ts` with `globals: true` and a `test/`/`src/**/*.test.ts` include. The tests will be pure unit tests that don't require a backend.
|
||
**Files to add:**
|
||
- `vitest.config.ts`
|
||
- `src/tools/grep.test.ts`
|
||
- `src/persistence/sessionStore.test.ts`
|
||
- `src/plugins/expandTemplate.test.ts` (small sanity test for existing behavior)
|
||
|
||
### 4. MCP content types: non-text results are placeholder-only
|
||
**Files:** `src/mcp/toolAdapter.ts`, `src/mcp/client.ts` (type update)
|
||
**Approach:**
|
||
- Update `McpToolInfo` and the call-tool handler to support `image`, `audio`, and `resource` content blocks from the MCP spec.
|
||
- For `image`: include `mimeType` and a truncated base64 note; if the model path is vision-capable, return the actual `data:` URI so the model can see it (same pattern as `read_file`).
|
||
- For `audio`: include `mimeType` and a note.
|
||
- For `resource`: if it's a text resource, inline the text; if binary, note the URI and mime type.
|
||
- Keep text blocks unchanged.
|
||
**Test:** Add `src/mcp/toolAdapter.test.ts` with mocked MCP content blocks of each type.
|
||
|
||
### 5. Hooks: only 6 events; no structured JSON output; no HTTP/prompt/agent hook types
|
||
**Files:** `src/hooks/types.ts`, `src/hooks/runner.ts`, `src/hooks/config.ts`
|
||
**Approach (scoped but complete within reason):**
|
||
- Expand the hook event set to the most useful missing events: `PermissionRequest`, `SubagentStart`, `SubagentStop`, `CwdChanged`, `FileChanged`, `ConfigChange`, `Stop` is already present. Final set: all 12 events from Claude Code's common set:
|
||
`SessionStart`, `UserPromptSubmit`, `PreToolUse`, `PostToolUse`, `PermissionRequest`, `SubagentStart`, `SubagentStop`, `CwdChanged`, `FileChanged`, `ConfigChange`, `Stop`, `SessionEnd`.
|
||
- Add typed payload shapes for each event in `src/hooks/types.ts`.
|
||
- Add a structured output schema option: hooks can specify `outputSchema: "json"` in their config; when set, stdout is parsed as JSON and validated against a simple zod schema. If parsing fails, treat as a warning.
|
||
- Add an `HTTPHook` type (simple GET/POST with optional headers/body) and a `PromptHook` type (shows a yes/no prompt to the user). Wire `HTTPHook` in the runner; `PromptHook` will be added to types but marked as not yet implemented in the runner to avoid UI-blocking complexity in this pass.
|
||
- Fire `PermissionRequest` right before the user confirm gate in `agent/loop.ts gateAndRun`.
|
||
- Fire `SubagentStart`/`SubagentStop` around `runSubAgentTurn`.
|
||
- Fire `CwdChanged` when the session `cwd` would change (currently static; hook is informational for future use).
|
||
- Fire `FileChanged` after a mutating `write_file`/`edit_file`/`bash` succeeds.
|
||
- Fire `ConfigChange` when `/model`, `/backend`, `/mode`, or `locode config set` changes config.
|
||
**Test:** Add `src/hooks/runner.test.ts` testing exit-code semantics (0/2/other), JSON output parsing, and tool-scoped matching.
|
||
|
||
### 6. Plugin collisions: MCP and skill name collisions are not resolved cleanly
|
||
**Files:** `src/mcp/config.ts`, `src/plugins/skillTool.ts`, `src/plugins/registry.ts`
|
||
**Approach:**
|
||
- MCP servers: when a plugin provides a server whose name collides with a user or project server, log/record the collision and keep the more-specific tier (project > user > plugin). Add a `collisions` field to the returned merge result and surface it in `/mcp`.
|
||
- Skills: namespace skills internally as `<pluginName>/<skillName>` in the skill tool, but still accept the bare skill name for the `/name` shortcut. If two skills share a bare name, prefer the first loaded plugin and emit a warning in `/skills` listing duplicates.
|
||
- Plugin agent names are already namespaced (`agent__<plugin>__<agent>`); commands are already namespaced by the fact that they share a single command namespace. Add a warning list for duplicate command names too.
|
||
- Add a `PluginCollisionWarning` type and expose it via `getLoadedPlugins()` metadata.
|
||
**Test:** Add `src/plugins/skillTool.test.ts` and `src/mcp/config.test.ts` for collision behavior.
|
||
|
||
### 7. Skill references: cannot load sibling reference files for a skill
|
||
**Files:** `src/plugins/loader.ts`, `src/plugins/types.ts`, `src/plugins/skillTool.ts`
|
||
**Approach:**
|
||
- When loading a skill, also read any `references/*.md` files in the same skill directory.
|
||
- Store them as `references: { name, content }[]` on `PluginSkill`.
|
||
- When the skill is invoked (via the `skill` tool or `/name`), concatenate the references after the main SKILL.md body under a clear header so the model sees them.
|
||
**Test:** Add `src/plugins/loader.test.ts` with a temporary in-memory plugin layout.
|
||
|
||
### 8. Git operations: `git_commit` covers a subset
|
||
**File:** `src/tools/git.ts`
|
||
**Approach:** Extend `git_commit` with these additional operations:
|
||
- `reset` — `git reset` (mixed by default) with optional `ref` and `mode` (soft/mixed/hard). Requires confirmation; preview shows affected commits/files.
|
||
- `stash` — `git stash push` (with optional message and paths) and `git stash pop` (with optional stash ref). Preview shows what will be stashed/popped.
|
||
- `merge` — `git merge <branchName>` with optional `--no-ff`/`--ff-only`. Preview shows branches and commits.
|
||
- `rebase` — `git rebase <branchName>` with optional `--onto`. Preview shows commits.
|
||
- `delete_branch` — `git branch -d/-D <branchName>`. Preview shows the branch and whether it has unmerged commits.
|
||
- Add `operation` union entries and the necessary parameters (`ref`, `mode`, `stashRef`, `branchName`, `strategy`, `message`, `paths`, `force`).
|
||
- Keep the existing preview/handler pattern.
|
||
**Test:** Add `src/tools/git.test.ts` that validates argument generation for each operation (no real git exec).
|
||
|
||
### 9. Context window: 85% auto-compact threshold is hardcoded
|
||
**Files:** `src/agent/loop.ts`, `src/config/config.ts`, `src/config/store.ts`, `src/config/defaults.ts`, `src/cli.ts`
|
||
**Approach:**
|
||
- Add `autoCompactThreshold` to `StoredConfig` and env var `LOCODE_AUTO_COMPACT_THRESHOLD`.
|
||
- Default remains 0.85; allow values 0.1–0.95.
|
||
- Add `locode config set autoCompactThreshold <0.0-1.0>` and `locode config get autoCompactThreshold`.
|
||
- Read it in `resolveAutoCompactThreshold()` and use it in `shouldAutoCompact` in `agent/loop.ts`.
|
||
- Pass the threshold into the `Session` object so sub-agents inherit it.
|
||
**Test:** Add `src/config/config.test.ts` for threshold resolution and `src/agent/loop.test.ts` for the compact check.
|
||
|
||
## Files to modify
|
||
1. `src/tools/grep.ts`
|
||
2. `src/persistence/sessionStore.ts`
|
||
3. `src/mcp/toolAdapter.ts`
|
||
4. `src/mcp/client.ts`
|
||
5. `src/mcp/types.ts` (add status/collision type)
|
||
6. `src/mcp/config.ts`
|
||
7. `src/mcp/manager.ts` (surface collision warnings)
|
||
8. `src/hooks/types.ts`
|
||
9. `src/hooks/runner.ts`
|
||
10. `src/hooks/config.ts`
|
||
11. `src/agent/loop.ts` (fire new hooks, use threshold)
|
||
12. `src/agent/session.ts` (store threshold)
|
||
13. `src/plugins/loader.ts`
|
||
14. `src/plugins/types.ts`
|
||
15. `src/plugins/skillTool.ts`
|
||
16. `src/plugins/registry.ts` (collision tracking)
|
||
17. `src/plugins/agentTool.ts` (add command dup tracking if needed)
|
||
18. `src/tools/git.ts`
|
||
19. `src/config/config.ts`
|
||
20. `src/config/store.ts`
|
||
21. `src/config/defaults.ts`
|
||
22. `src/config/types.ts`
|
||
23. `src/cli.ts` (add config key)
|
||
24. `src/ui/ink/App.tsx` (pass threshold, fire ConfigChange)
|
||
25. `src/ui/ink/HistoryItemView.tsx` (show MCP collision warnings)
|
||
26. `README.md` (update limitations)
|
||
|
||
## Files to add
|
||
1. `vitest.config.ts`
|
||
2. `src/tools/grep.test.ts`
|
||
3. `src/persistence/sessionStore.test.ts`
|
||
4. `src/plugins/expandTemplate.test.ts`
|
||
5. `src/mcp/toolAdapter.test.ts`
|
||
6. `src/hooks/runner.test.ts`
|
||
7. `src/plugins/skillTool.test.ts`
|
||
8. `src/mcp/config.test.ts`
|
||
9. `src/plugins/loader.test.ts`
|
||
10. `src/tools/git.test.ts`
|
||
11. `src/config/config.test.ts`
|
||
12. `src/agent/loop.test.ts`
|
||
|
||
## Validation
|
||
- Run `npm run typecheck` — must pass.
|
||
- Run `npm test` — must pass.
|
||
- Run `npm run build` — must succeed.
|
||
|
||
## Risks / trade-offs
|
||
- Expanding hooks to 12 events touches `agent/loop.ts` in several places; need to keep event payloads consistent.
|
||
- MCP content type support is best-effort; real vision models may still ignore audio/resource blocks.
|
||
- Git operation expansion increases the chance of destructive commands (`reset --hard`, `rebase`, `delete_branch`); previews must be clear and the tool remains mutating/confirm-gated.
|
||
- Making the auto-compact threshold configurable requires threading a new field through `Session` creation/resumption.
|