Skip to content

Commit 2248814

Browse files
bug-opsbug-ops
committed
chore: add Copilot custom instructions for code review
Repository-wide instructions (.github/copilot-instructions.md) cover architecture overview, lock scope rules, glob anchoring, channel semantics, and deny.toml license gaps. Path-specific instruction files cover: - bridge/lsp: TOCTOU, version overflow, notification_pump deadlock, file watcher - config/deps: license allow list, serde round-trip tests, TOML defaults - mcp: 1-based positions, JsonSchema derive, tool dispatch completeness - tests: #[ignore] hygiene, resync coverage, glob pattern correctness - ci: feature flag parity, RUSTDOCFLAGS, MSRV guards - CHANGELOG: breaking change format, PR references Instructions are derived from findings in PR #101 and #103 reviews.
1 parent 522bad5 commit 2248814

7 files changed

Lines changed: 237 additions & 0 deletions

‎.github/copilot-instructions.md‎

Lines changed: 58 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,58 @@
1+
# Copilot Instructions for mcpls
2+
3+
mcpls is a bridge between MCP (Model Context Protocol) and LSP (Language Server Protocol).
4+
AI clients speak MCP to mcpls; mcpls spawns and communicates with language servers over LSP.
5+
6+
## Architecture
7+
8+
```
9+
AI Client ←→ [MCP/stdio] ←→ mcpls ←→ [LSP/stdio] ←→ rust-analyzer / tsgo / pyright / ...
10+
```
11+
12+
Key crates:
13+
- `mcpls-core/src/bridge/` — MCP→LSP translation, document state, diagnostics cache
14+
- `mcpls-core/src/lsp/` — LSP client, JSON-RPC 2.0 lifecycle, file watcher
15+
- `mcpls-core/src/mcp/` — MCP server, tool definitions and dispatch
16+
- `mcpls-core/src/config/` — TOML config, LSP server discovery heuristics
17+
18+
## Code Review Priorities
19+
20+
### Correctness
21+
22+
- **Position encoding**: MCP is 1-based, LSP is 0-based. All conversions must go through
23+
`bridge/encoding.rs`. Flag any direct arithmetic on line/column values outside that module.
24+
25+
- **LSP protocol compliance**: Inbound JSON-RPC messages fall into four shapes:
26+
`method`+`id` → server request, `method` only → notification,
27+
`id`+(`result`|`error`) → response, anything else → protocol error.
28+
Responses without `result` or `error` must not be silently treated as `null`.
29+
30+
- **Document versioning**: `textDocument/didOpen` version numbers must be strictly
31+
monotone per document URI. After `didClose`/`didOpen` resync cycles, reset to 1
32+
rather than saturating at `i32::MAX`.
33+
34+
- **Glob patterns**: `globset` matches against the full absolute path. Bare patterns
35+
like `*.rs` only match filenames with no directory component. Patterns without a
36+
leading `/` or `**/` prefix must be anchored with `**/` before passing to `GlobSetBuilder`.
37+
38+
### Concurrency
39+
40+
- **Lock scope**: `Arc<Mutex<Translator>>` must never be held across async LSP I/O
41+
(notify calls, request round-trips). If a lock guard is held while awaiting a network
42+
operation, flag it — this creates head-of-line blocking between request handlers and
43+
the `notification_pump`.
44+
45+
- **Channel semantics**: `std::sync::mpsc::SyncSender::send` blocks when the channel is
46+
full; it does not drop. Use `try_send` when the intent is drop-on-full.
47+
48+
### Dependencies
49+
50+
- Check `deny.toml` allow list when new crates are introduced. Licenses not in the list
51+
will fail `cargo deny check licenses` in CI. Common gap: `CC0-1.0` (used by `notify`).
52+
53+
## Style
54+
55+
- All `pub` types, traits, functions, and methods must have `///` doc comments explaining
56+
*what* and *why*, not just restating the name.
57+
- No `unsafe` code — `deny(unsafe_code)` is enforced workspace-wide.
58+
- `#[non_exhaustive]` is required on any public enum that may gain variants in future releases.

‎.github/instructions/changelog.instructions.md‎

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,14 @@
1+
---
2+
applyTo: "CHANGELOG.md"
3+
---
4+
5+
## CHANGELOG review checklist
6+
7+
- Breaking changes to public APIs (new enum variants, removed fields, changed
8+
signatures) must appear under `### Changed` with an explicit "Breaking change:"
9+
prefix. A `### Fixed` entry alone is not sufficient.
10+
- New `#[non_exhaustive]` enums or structs must be mentioned: downstream crates
11+
that match exhaustively need to add a wildcard arm.
12+
- Every entry must reference the PR or issue number in parentheses: `(#NNN)`.
13+
- Entries go under `## [Unreleased]` until a release PR assigns a version.
14+
Do not add entries directly under a versioned section.

‎.github/instructions/ci.instructions.md‎

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,30 @@
1+
---
2+
applyTo: ".github/workflows/**,deny.toml,.cargo/**"
3+
---
4+
5+
## CI and toolchain review checklist
6+
7+
### Workflows
8+
9+
- Feature flag sets in CI jobs must match the local pre-commit commands in `CLAUDE.md`
10+
exactly: `--all-features` for clippy and nextest, `--no-deps --all-features` for
11+
rustdoc. Divergence causes "passes locally, fails CI" issues.
12+
- `RUSTDOCFLAGS="-D warnings"` must be set in the rustdoc job. Without it, broken
13+
intra-doc links and missing doc comments pass silently.
14+
- Nightly toolchain is required only for `cargo +nightly fmt`. All other jobs must use
15+
stable. Pinning nightly globally causes unnecessary breakage on nightly regressions.
16+
17+
### deny.toml
18+
19+
- New dependencies require a license check before merge. Add the license to the
20+
`allow` list in the same PR that introduces the crate. Do not merge a PR that
21+
introduces a new crate without verifying `cargo deny check licenses` passes.
22+
- Known unlisted licenses that will fail CI: `CC0-1.0` (used by `notify`).
23+
- `cargo deny check advisories` must be clean. A new RUSTSEC advisory in a transitive
24+
dependency is a blocker even if the vulnerable code path is not exercised.
25+
26+
### MSRV
27+
28+
- `rust-version` in `Cargo.toml` is the enforced minimum. Any API stabilised after
29+
that version requires either a version bump (document in CHANGELOG) or a conditional
30+
compile guard.

‎.github/instructions/mcp-tools.instructions.md‎

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,25 @@
1+
---
2+
applyTo: "crates/mcpls-core/src/mcp/**"
3+
---
4+
5+
## MCP layer review checklist
6+
7+
### Tool parameter structs (`tools.rs`)
8+
9+
- Every position parameter (`line`, `character`) must be documented as 1-based in both
10+
the `///` doc comment and the `#[schemars(description)]` annotation. LSP is 0-based;
11+
the conversion happens in `bridge/encoding.rs` — not here.
12+
- `file_path` parameters must accept absolute paths only. Doc comments must state this
13+
explicitly so AI clients don't pass relative paths.
14+
- New tool structs require `#[derive(JsonSchema)]` — omitting it silently removes the
15+
tool from the MCP schema exposed to clients.
16+
17+
### Tool dispatch (`handlers.rs`, `server.rs`)
18+
19+
- Every tool handler must call `ensure_open` (via `Translator`) before issuing any LSP
20+
request. Skipping it produces stale results when the file has changed on disk since
21+
the last access.
22+
- Tool errors must propagate as MCP error responses, not panics. `unwrap()` and
23+
`expect()` are not acceptable in handler code.
24+
- New tools must be registered in both the tool list and the dispatch match arm.
25+
A tool missing from either will either never appear to clients or panic at runtime.

‎.github/instructions/rust-bridge.instructions.md‎

Lines changed: 55 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,55 @@
1+
---
2+
applyTo: "crates/mcpls-core/src/bridge/**,crates/mcpls-core/src/lsp/**,crates/mcpls-core/src/lib.rs"
3+
---
4+
5+
## Bridge and LSP layer review checklist
6+
7+
### Document state (`bridge/state.rs`)
8+
9+
- `ensure_open` stats the file on every call and compares `(mtime, size)` against the
10+
cached `SyncSignature`. On mismatch it must send `didClose` then `didOpen` with a
11+
bumped version before returning. Verify this resync path is exercised by a test.
12+
- Stat must happen on the same `File` handle as the subsequent read, or be re-done after
13+
the read, to avoid TOCTOU: an atomic `rename(2)` between stat and `read_to_string` on
14+
a coarse-mtime filesystem (ext4: 1 s, FAT32: 2 s) leaves the cached signature stale
15+
permanently.
16+
- Version counter must reset to 1 on overflow, not saturate. `saturating_add(1)` at
17+
`i32::MAX` sends the same version on every subsequent resync; rust-analyzer silently
18+
discards non-monotone `didOpen` notifications.
19+
20+
### Diagnostics pipeline (`bridge/translator.rs`, `bridge/notifications.rs`)
21+
22+
- `DiagnosticsMode::Cached` returns an empty `Vec` for both "no errors" and "server
23+
has not yet analysed this file". AI clients cannot distinguish these. Flag if callers
24+
treat empty as "clean".
25+
- `merge_diagnostics` dedup key is `(range, severity, code)` when `code` is present,
26+
falling back to a path-qualifier-stripped message otherwise. Doc comments that say
27+
`(range, message, code)` are wrong — flag them.
28+
- `pull_diagnostics` has a 30 s timeout. In `Hybrid` mode it runs before the cache read.
29+
The cache read is a synchronous hashmap lookup; it should not be blocked behind the
30+
pull. Prefer `tokio::join!` for the two sources.
31+
32+
### Notification pump (`lib.rs`)
33+
34+
- `notification_pump` must hold only a `NotificationCache` lock, never the full
35+
`Translator` lock. Holding `Mutex<Translator>` across an LSP round-trip in a request
36+
handler while the pump waits for the same lock causes head-of-line deadlock when the
37+
notification channel fills.
38+
39+
### File watcher (`lsp/file_watcher.rs`)
40+
41+
- `GlobPattern::String` patterns without a leading `/` or `**/` must be anchored with
42+
`**/` before passing to `globset`. Bare `*.rs` does not match `/repo/src/lib.rs`.
43+
- `SyncSender::send` blocks when the channel is full. Use `try_send` with a warn log
44+
when the documented intent is drop-on-full.
45+
- `NEVER_FORWARD_COMPONENTS` filtering should happen before the channel send, not after,
46+
to avoid burning channel capacity on `target/` churn during `cargo build`.
47+
- `register()` overwrites duplicate registration IDs silently. LSP spec treats
48+
re-registration as an error; at minimum log a warning.
49+
50+
### LSP client (`lsp/client.rs`, `lsp/transport.rs`)
51+
52+
- Server-to-client requests (`method` + `id`) must receive a JSON-RPC response.
53+
Unhandled methods should return error code `-32601` (Method Not Found), not be dropped.
54+
- `id`-only messages (no `method`, no `result`, no `error`) are protocol errors and must
55+
not be treated as successful `null` responses.

‎.github/instructions/rust-config.instructions.md‎

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,22 @@
1+
---
2+
applyTo: "crates/mcpls-core/src/config/**,deny.toml,Cargo.toml,crates/*/Cargo.toml"
3+
---
4+
5+
## Config and dependency review checklist
6+
7+
### deny.toml
8+
9+
- When a new crate is introduced, verify its license appears in the `allow` list.
10+
`cargo deny check licenses` runs in CI and will fail on first run if the license is
11+
missing. Known gap: `CC0-1.0` (used by `notify`).
12+
- Check `cargo deny check advisories` output for any new RUSTSEC advisories in added
13+
dependencies.
14+
15+
### Config (`config/mod.rs`)
16+
17+
- New `#[serde(default)]` fields on public config structs must have a test covering
18+
the omitted-key case (TOML round-trip, not just JSON).
19+
- Enum config options must include serde round-trip tests for every variant, both JSON
20+
and TOML, since `#[serde(rename_all)]` applies to both serializers independently.
21+
- LSP server discovery heuristics (file-pattern matching) must be case-insensitive on
22+
the extension comparison — APFS and NTFS are case-insensitive filesystems.

‎.github/instructions/tests.instructions.md‎

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,33 @@
1+
---
2+
applyTo: "crates/mcpls-core/tests/**,crates/mcpls-core/src/**/tests.rs,crates/mcpls-core/src/**/*_tests.rs"
3+
---
4+
5+
## Test review checklist
6+
7+
### Integration tests (`tests/integration/`)
8+
9+
- Tests requiring a live LSP binary (`rust-analyzer`, `tsgo`, etc.) must be marked
10+
`#[ignore = "Requires <binary> in PATH"]`. Unmarked tests that shell out to LSP
11+
binaries will break CI on clean runners.
12+
- `rust_analyzer_tests.rs` probes must verify the post-PR #103 diagnostic pipeline:
13+
`get_diagnostics` should return errors for files with known type errors, and
14+
`get_cached_diagnostics` should return the same set (non-empty) after a
15+
`publishDiagnostics` notification has been received.
16+
17+
### E2e tests (`tests/e2e/`)
18+
19+
- Every e2e test spawns the mcpls binary. If the binary is not pre-built, the test
20+
must be `#[ignore]`. Document the required build step in the ignore message.
21+
- Position values in e2e assertions must use 1-based line/column (MCP convention).
22+
Using 0-based values will produce off-by-one failures that are hard to diagnose.
23+
24+
### Unit tests
25+
26+
- `ensure_open` resync path (signature mismatch → `didClose` + `didOpen`) must be
27+
covered by an async test using a temp file and a mock `LspClient`. The stat-only
28+
test (`test_stat_signature_changes_when_file_grows`) does not cover this path.
29+
- `FileWatcher::register` and `compute_changes` require unit tests. New glob
30+
registrations must be verified to actually match the intended file paths using
31+
absolute path strings — bare patterns like `*.rs` will not match.
32+
- `DiagnosticsMode` serde tests must cover all three variants in both JSON and TOML
33+
deserialization, including the omitted-key default.

0 commit comments

Comments
 (0)