Address static audit: protocol, security, position encoding, indexing, and correctness#497
Conversation
Work through the static audit's P0/P1/P2 findings: Security - Replace shell command strings with argv-based os.Process execution (no shell, so filenames/paths can't inject commands). - Set child working dir per-process instead of a global chdir. - Bind TCP to loopback by default; non-loopback needs --unsafe-allow-remote. Protocol / transport - Support string request/cancel IDs (raw-id extraction + verbatim echo). - Consume responses to server-initiated requests instead of dispatching them. - Enforce request/notification direction. - Frame limits: max Content-Length, header cap, real buffer size, non-UTF-8 charset rejection; surface header errors before allocating. - Proper file: URI percent encode/decode, UNC/drive/fragment handling. Document synchronization - Lossless incremental edits that preserve CRLF/CR and final newline. - Monotonic version enforcement; ignore edits to unopened documents. - willSaveWaitUntil no longer mutates server state. - File watcher never overwrites unsaved open buffers. - Clear diagnostics on close; publish diagnostics on open. - Distinguish empty vs missing document on save. Edits / correctness - Range formatting reads the rewritten file and only edits a hunk fully inside the requested range. - Organize Imports refuses non-contiguous import blocks and honors context.only; unknown-import quick fix removes the whole line. - Rename requires a resolved semantic anchor (no lexical fallback). - Fix UTF-8 byte-slicing at word boundaries; guard negative positions. Diagnostics / performance - Per-project cache invalidation instead of a global generation. - Keep distinct diagnostics at the same position; clamp negative positions. - Shallow sibling scan instead of recursive tree walk. - Logging is opt-in via VLS_LOG (no source leakage / file churn by default). Capabilities - Stop advertising placeholder features (on-type formatting, inline values, linked editing, file-operation hooks, run/test commands, CodeLens stubs); completion no longer triggers on space. Tests updated to assert the corrected behavior, plus new regression tests for URIs, framing, string IDs, direction, CRLF edits, and safe organize imports. compilation_test.v is now a real smoke test.
- Switch all production and test files from the deprecated `json` module to `json2` (json.decode(T, s) -> json2.decode[T](s), encode options). - Decode inbound requests via an id-less RequestBody: json2 aborts the whole decode on a string value in an int field, so the numeric id is derived from the raw id token and the exact id is echoed separately. - Cancellation captures the raw id before the int decode for the same reason. - Add a custom to_json() encoder for the self-referential SelectionRange, which json2's comptime encoder cannot derive from `?&SelectionRange`. Build is warning-free and `v fmt -verify` is clean, so `v fmt` no longer tries (and fails) to auto-migrate the files.
Position encoding (P0-01) - Add a PositionCodec: encoded_col_to_byte / byte_to_encoded_col convert between LSP character offsets (UTF-16 default, UTF-8, or UTF-32) and byte offsets. Non-BMP characters correctly count as two UTF-16 units. - Negotiate the encoding at initialize (prefer UTF-8 when offered, else the mandatory UTF-16) and advertise it via the positionEncoding capability. - Route client positions through the codec: incremental edits, word-boundary lookups (rename/highlight/linked/call hierarchy), and the compiler line-info column. The V compiler works in byte columns, so convert client->byte on the way in and byte->client (via the target line) on the way out. - Table-driven codec tests (ASCII, BMP, non-BMP, combining marks, out-of-range) and a real end-to-end check that go-to-definition across an emoji resolves. Process robustness (partial P0-04) - Kill any compiler/formatter child that exceeds compiler_timeout_ms (default 30s, VLS_TIMEOUT_MS override) so a hung `v` can't freeze the server. Compiler failure surfacing (P1-03) - Warn the client via window/showMessage when the V compiler is not on PATH, instead of silently returning empty diagnostics/completion/navigation.
Now that the server advertises a position encoding, re-encode every emitted character offset from bytes into that encoding (P0-01, P2-01, P2-07, P2-09): - Diagnostics: re-encode the compiler's byte columns per document line. - Semantic tokens: convert token start/length to the negotiated encoding, and return an empty token object (not null) for empty documents. - Full formatting: end-of-document character uses the encoded line length. - Inlay hints: hint column re-encoded from the byte offset. - Selection range: full-line range end uses the encoded line length. Tests updated for the empty-document semantic-token behavior.
Clear the $/cancelRequest tracking maps when they exceed a bound, so a client sending cancellations for ids that never correspond to an in-flight request cannot grow them without limit (P0-04 note).
Add a project-scoped symbol index (audit Stage 4) so symbol-driven features stop re-walking and re-parsing the whole workspace on every request. - index.v: per-URI IndexEntry (hierarchical symbols + vdoc comments), keyed by document URI with a content fingerprint. Open buffers are indexed from memory (authoritative), unopened files from disk; unchanged content is not re-parsed. - Scope: the index only walks a real project (nearest v.mod, audit P1-01) or a configured workspace root, never an arbitrary file's parent directory, and the walk skips hidden/heavy dirs (.git, node_modules, ...) and is bounded in file count and size. This prevents unbounded traversal of a home dir or /tmp. - Incremental maintenance: didOpen/didChange/didSave refresh via fingerprint; didClose and file-watcher events re-index the affected URI from disk. - Migrated consumers to the index: workspace symbols (now includes tests, P2-11), document symbols, hover doc lookup (P1-08), and call-hierarchy prepare — each previously re-scanned the workspace per request. Also make the response `_type` sum-type-tag stripper generic (P1-10): array and future result variants can no longer leak the non-standard discriminator, which the now-functional workspace-symbol results exposed. Adds index_test.v (index build/query, incremental reindex, project-root scoping, walk exclusions) and array-variant `_type` stripping tests.
Move free-function ("same module") completion off the per-keystroke workspace
walk and onto the persistent index (P2-06). The index now stores each file's
function completion items; completion ensures the open buffers and the current
file's directory are indexed (a V module is a single directory, so a cheap
shallow scan suffices) and reads pre-parsed items filtered by module, instead of
recursively walking and re-parsing sibling files on every keystroke.
Add a per-file identifier-occurrence index (name -> positions, in the negotiated encoding, cached by content fingerprint) and drive references/rename from it instead of re-walking, re-reading, and re-tokenizing the workspace on every request (P1-05): - search_symbol_in_dirs and search_symbol_in_dirs_semantic now read candidate occurrences from the index, scoped to the v.mod/workspace project like the symbol index (no unbounded walk). The semantic path still validates each candidate against the compiler definition anchor, so references remain scope-safe across same-name symbols in different scopes/modules (P1-04). - resolve_symbol_anchor now converts the client-encoding column to the byte column the compiler expects, fixing anchor resolution after non-ASCII text (P0-01). - Remove the dead search_symbol_in_project wrapper and the per-request lexical workspace re-scan. Verified end to end: references on a function in a v.mod project returns the declaration plus every call site across files, with no _type leak. Adds tests for occurrence extraction (skips comments/strings/numbers), fingerprint caching, and cross-file lexical search.
Document highlights previously returned every occurrence as kind Text. Classify each occurrence as Write (assignment target: `x :=`, `x =`, `x += ...`) or Read (comparisons, uses, declarations) via a syntactic heuristic, so editors can distinguish reads from writes (P2-03).
…sitions Framing (P0-11): - Centralize message framing; TCP always emits literal `\r\n\r\n` (Windows TCP no longer emits LF-only framing), while Windows stdio keeps `\n\n` for the text-mode translation. Close the connection after an unrecoverable framing error instead of continuing (the unread body would desync the stream). Position encoding (P0-01): - Re-encode document-symbol ranges into the negotiated encoding in the index, so document symbols and workspace symbols are correct for non-ASCII lines. - Thread the encoding through call hierarchy (prepare/incoming/outgoing) and the Organize Imports edit, replacing byte/code-point offsets.
Cancelling a string id (e.g. "a") previously also inserted numeric id 0 into the cancellation map (via raw_id_to_int), so it spuriously cancelled numeric id 0 and every other string id collided at 0 (P0-02). Track string ids only in the raw-id set, and match/consume string-id requests exclusively by their exact raw id.
…offsets - Stop advertising documentRangeFormattingProvider: v fmt only formats whole files, so range formatting needs a character-accurate, EOL-preserving diff restricted to the requested range, which is not yet implemented (P0-08). - Full formatting now computes the document end position from line-start byte offsets instead of split_into_lines(), which dropped the empty line after a trailing newline and left the final terminator outside the replacement.
…o_uri - didSave for a document that is not open no longer inserts it into open_files (which left it logically editor-owned forever); it only computes diagnostics from the saved text or disk content. - Definition/declaration results build the target DocumentUri via path_to_uri (percent-encoded) instead of concatenating `file://`, so paths with spaces, `#`, `%`, or Unicode are valid and match the client's URI keys (P0-10).
…rgets - collect_v_files now tracks canonical (realpath) directories, so a symlink cycle can't cause infinite recursion or double-indexing. - Rename validation rejects a newName that is a V keyword or builtin, which would otherwise produce uncompilable code.
…version An incremental change with an invalid (negative/reversed/out-of-bounds) range was applied as a no-op while the version was still advanced, silently desynchronizing the buffer. on_did_change now validates the range first and, if it cannot be applied, refuses the whole change and keeps the last-good content and version (P0-07).
run_v_line_info (single-file compile) and the hover doc lookup fell back to the global App.text — the last-touched buffer — when the requested URI was neither open nor readable, so a request for one document could use another document's content. Both now fall back to the exact file on disk, or empty, never to a different buffer (P1-02).
workspace/didChangeWorkspaceFolders removal now clears the symbol/occurrence index entries under the removed folder and forces a re-walk, so a removed folder's symbols do not linger. Still-open files are re-indexed from their buffers on the next query.
The argv-based run_v_argv returned only the child's stdout, but `v -check -json-errors` (and -line-info) write their JSON to STDERR. As a result every diagnostic was parsed from an empty string and dropped — the server published empty diagnostics for files with real errors. Merge both streams into the returned output (equivalent to the previous shell `2>&1`). Verified end to end: `undefined ident` is now reported, and go-to-definition still resolves.
Guards against the diagnostics regression where the compiler's stderr JSON was dropped and the server published an empty list for files with real errors.
Go-to-definition and signature-help tests previously asserted only the response id, so they stayed green even when the compiler-interop path returned null (as happened when compiler stderr was dropped). Assert the resolved Location/SignatureHelp content — definition points at the decl line, signature help carries the function label — so these paths cannot silently regress to null again.
…request handle_inlay_hints rebuilt its function-return-type index on every request by walking and re-reading every imported vlib module directory from disk. vlib source is immutable for the life of a session, so this repeated walk+read+parse was pure waste on a request that fires on every scroll/edit. Cache the per-module vlib index on the App (keyed by module name); build it once on first use and merge from the cache thereafter. Project and open files are still re-read live since they change mid-session. Missing modules cache an empty index so they are not re-walked either.
|
@codex review |
1 similar comment
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Lines 1528 to 1529 in 95063dd
Reject whitespace around rename targets
When newName contains leading or trailing whitespace, validation succeeds because it checks the trimmed value, but handle_rename inserts the original untrimmed string into every edit. Inputs such as " renamed" therefore produce uncompilable V code; either require name == name.trim_space() or return and use the validated normalized name.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // the child against a full pipe buffer, and enforce the timeout. | ||
| for p.is_alive() { | ||
| mut got_data := false | ||
| if chunk := p.pipe_read(.stdout) { |
There was a problem hiding this comment.
When the compiler writes only to stderr or hangs before producing stdout, the blocking stdout pipe_read prevents execution from reaching either the stderr drain or the timeout check. A sufficiently large stderr payload can therefore fill its pipe and deadlock the child, while a silent hung child bypasses the intended 30-second kill entirely; drain both streams concurrently or use nonblocking reads while enforcing the deadline.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 842c587. pipe_read is non-blocking here: it polls the fd (fd_is_pending -> ioctl(FIONREAD) / zero-timeout select) and returns none immediately when no data is pending, so the loop already drains both pipes and checks the deadline on every iteration. A stderr-only or silently-hung child can therefore neither fill a pipe and deadlock nor bypass the 30s kill. The stale doc comment (it still said stderr was "only logged") is what invited the concern; I updated it to describe the merged-stream, non-blocking-drain design.
| if dir == '' || dir == '/' || dir in app.indexed_dirs || !os.is_dir(dir) { | ||
| continue |
There was a problem hiding this comment.
When a client does not support dynamic watched-file registration, rejects the registration, or otherwise sends no watcher notifications, every directory is skipped after its first index walk. Consequently, newly created unopened files are never discovered and changed unopened files retain stale workspace-symbol, hover, and call-hierarchy data until the server restarts; these directories need a refresh strategy when watcher-driven invalidation is unavailable.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 89eb05b. When the client does not support dynamic watched-file registration, indexed directories are now re-walked on a throttled interval (index_refresh_interval_ms, 10s) instead of once-and-never, so newly created and edited unopened files are discovered; the per-file fingerprint check keeps a refresh walk from re-parsing unchanged files. When watchers are available we still rely on didChangeWatchedFiles.
| dir := os.dir(uri_to_path(uri)) | ||
| app.project_generations[dir] = app.project_generations[dir] + 1 |
There was a problem hiding this comment.
When an open file in an imported module's directory changes, this increments only that exact directory's generation, while run_v_check validates an importing file's cache against the importing directory's generation. A later save or diagnostics request for unchanged importing source can therefore reuse results computed against the old imported API—for example, after a referenced function is removed—so invalidation needs to cover dependent project directories rather than only os.dir(uri).
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 89eb05b. bump_generation/project_generation now key on the enclosing v.mod project root instead of os.dir(uri), so editing an imported module in a sibling directory invalidates cached diagnostics for importers in the same project. Files outside any project fall back to their immediate directory.
| struct CodeActionContext { | ||
| diagnostics []LSPDiagnostic | ||
| diagnostics []LSPDiagnostic | ||
| only ?[]string @[json: 'only'] |
There was a problem hiding this comment.
only
The V field name only already exactly matches the JSON member name, so this annotation is redundant and violates the repository's explicit rule that @[json] attributes be used only when the names differ. Remove the attribute while retaining the optional field.
AGENTS.md reference: AGENTS.md:L16-L17
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 1efbb3c: removed the redundant @[json: 'only'] and kept the mapping on trigger_kind.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 95063dd3fa
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| struct CodeActionContext { | ||
| diagnostics []LSPDiagnostic | ||
| diagnostics []LSPDiagnostic | ||
| only ?[]string @[json: 'only'] |
There was a problem hiding this comment.
only
The new only field already exactly matches its JSON key, so @[json: 'only'] violates the repository's JSON-attribute convention; remove this attribute while retaining the required mapping on trigger_kind.
AGENTS.md reference: AGENTS.md:L16-L17
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 1efbb3c: removed the redundant @[json: 'only'] (the field name already matches the JSON key); kept @[json: 'triggerKind'] on trigger_kind.
| starts := line_start_offsets(content) | ||
| start_byte := position_to_byte_offset(content, starts, range.start.line, range.start.char, enc) | ||
| end_byte := position_to_byte_offset(content, starts, range.end.line, range.end.char, enc) | ||
| return start_byte <= end_byte && start_byte <= content.len && end_byte <= content.len |
There was a problem hiding this comment.
When a stale client sends an incremental edit whose start and end lines are past the document, position_to_byte_offset clamps both positions to content.len, so this condition returns true; on_did_change then inserts the edit at EOF and advances the version instead of rejecting it, corrupting the server's synchronized buffer. Check both line indices against starts.len before converting them.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in d825614. incremental_change_is_valid now rejects a range whose start or end line is >= line_start_offsets(content).len, so a stale client's past-EOF edit is refused (no version bump) instead of being clamped to EOF and appended.
| end := skip_json_value(content, i) | ||
| return content[i..end].trim_space() |
There was a problem hiding this comment.
For a request carrying an ID such as true, {}, or [], this returns the arbitrary raw value and the new RequestBody decode never type-checks it; the server consequently processes the request and echoes an invalid ID type in its response. JSON-RPC IDs are limited to strings, numbers, or null, so unsupported types should produce an Invalid Request response rather than enter normal dispatch.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 89eb05b. A present id that is not a string or number (true, {}, []) now returns Invalid Request (-32600) with a null response id instead of being dispatched. Verified end-to-end: "id":{} and "id":true both get the error while numeric/string ids still dispatch normally.
| end: Position{ | ||
| line: line_nr + 1 | ||
| char: 0 | ||
| } |
There was a problem hiding this comment.
When the unknown import is the final line of a file without a trailing newline, line_nr + 1 does not exist, so this quick fix emits an invalid LSP range and clients may reject the entire workspace edit. Use the next-line position only when a line terminator is present; otherwise end at the final line's encoded length.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in d825614. The Remove-unknown-import quick fix now ends the deletion at [line+1,0) only when the line has a terminator; for a final line with no trailing newline it ends at the line's encoded length, keeping the range in-bounds so clients accept the edit.
The doc comment still described the old behavior (capture stdout, log stderr only), which is what invited the review concern that a blocking stdout read could starve the stderr drain and the timeout. pipe_read is non-blocking (it polls the fd and returns none immediately when no data is pending), so both streams are drained every iteration and the deadline is always checked; document that so the merged-stream, non-blocking design is clear.
The field name already matches the JSON member `only`, so per AGENTS.md the @[json] attribute is only for names that differ. Keep the mapping on trigger_kind, which does differ (triggerKind).
Two range-bounds defects in the sync/code-action paths: - incremental_change_is_valid accepted a range whose start/end line was past EOF. position_to_byte_offset clamps out-of-range lines to content.len, so a desynced client's edit looked valid, got appended at EOF, and advanced the document version — silently desyncing the buffer. Reject a range whose start or end line does not exist. - The "Remove unknown import" quick fix always ended its deletion range at [line+1, 0). When the import is the final line with no trailing newline that line does not exist, producing an out-of-bounds range clients may reject. End at the next line only when a terminator is present, otherwise at the final line's encoded length.
Three server-correctness fixes flagged in review:
- Reject unsupported JSON-RPC request id types. An id of true/{}/[] was
returned raw by extract_raw_id and never type-checked, so the request was
dispatched and echoed an invalid id. Ids may only be a string, number, or
null; a present id of any other type now yields an Invalid Request (-32600)
with a null response id instead of entering dispatch.
- Scope diagnostic-cache invalidation to the project root, not os.dir(uri).
bump_generation keyed off the immediate directory, so editing an imported
module in a sibling directory did not invalidate an importing file's cached
diagnostics — a stale result computed against the old imported API could be
reused. Key the generation on the enclosing v.mod root so a change anywhere
in the project invalidates dependents.
- Refresh indexed directories when client file watchers are unavailable.
Without dynamic watched-file registration every directory was walked once and
never revisited, so newly created and edited unopened files stayed invisible
or stale until restart. Re-walk on a throttled interval (fingerprint check
skips unchanged files) when watchers are absent; trust watcher events when
present.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ce76e88e7d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // with disk content — doing so would erase unsaved edits (P0-07 | ||
| // item 8). We only invalidate caches so future checks re-read | ||
| // non-open files from disk. | ||
| if uri in app.open_files { |
There was a problem hiding this comment.
When a watcher reports an equivalent URI with a different spelling—such as file:///tmp/a.v for an open file://localhost/tmp/a.v, or different drive-letter casing on Windows—this raw-key lookup misses the authoritative open buffer and the else branch indexes stale disk content under a second URI. With acknowledged watchers, the shallow completion refresh does not reconcile duplicates, so the disk-backed copy can contribute stale same-module completions despite unsaved edits. Fresh evidence beyond the earlier walked-file fix is that this watcher path still uses raw URI membership; resolve the event URI against normalized open-file paths and reuse the client's URI before reindexing.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4b79ea17cf
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if app.symbol_index.len >= index_max_files { | ||
| app.index_incomplete_scopes[scope] = true | ||
| break |
There was a problem hiding this comment.
When a watcher-less shallow refresh encounters a new file after the global index has reached index_max_files, this breaks before the remaining directory entries are added to present, but the reconciliation below still deletes every unseen existing entry in that directory. Thus adding one sibling to a full index can make valid completion/reference symbols disappear. Fresh evidence beyond the earlier recursive-walk finding is that this shallow path still reconciles after its own partial scan; track scan completeness and skip deletion reconciliation when the cap is hit.
Useful? React with 👍 / 👎.
| bind_host := if host == '' { '127.0.0.1' } else { host } | ||
| addr := '${bind_host}:${port}' |
There was a problem hiding this comment.
When TCP mode is started with the explicitly accepted loopback host --host ::1, this constructs ::1:<port> rather than the required bracketed host/port form [::1]:<port>. The listener therefore cannot parse or bind the advertised IPv6 loopback configuration; format IPv6 literals with brackets before passing the address to net.listen_tcp.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2d0c148ca5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| target_uri := path_to_uri(uri_path) | ||
| client_col := app.byte_col_to_client_col(target_uri, line_nr, col) |
There was a problem hiding this comment.
When compiler output points to an open buffer whose client URI is equivalent but noncanonical, such as file://localhost/tmp/a.v or differently cased Windows paths, rebuilding target_uri misses the authoritative open_files entry. byte_col_to_client_col then converts the compiler's byte column using stale disk content, so an unsaved non-ASCII edit before the declaration produces an incorrect UTF-16/UTF-32 location, and the response also changes the URI spelling; resolve the mapped path back to its open URI before converting and returning it.
Useful? React with 👍 / 👎.
| // Compound assignment: += -= *= /= %= &= |= ^= (op followed by '=', not '=='). | ||
| if rest.len >= 2 && rest[1] == `=` && (rest.len < 3 || rest[2] != `=`) | ||
| && rest[0] in [`+`, `-`, `*`, `/`, `%`, `&`, `|`, `^`] { | ||
| return doc_highlight_write |
There was a problem hiding this comment.
When a highlighted variable is mutated with count++ or count--, this classifier falls through to doc_highlight_read because it only recognizes assignment operators containing =. Editors consequently label common mutations as reads, making read/write highlighting inaccurate; explicitly recognize V's increment and decrement operators as writes.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 42ca55cf2a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| for uri, _ in app.open_files { | ||
| root := find_project_root(os.dir(uri_to_path(uri))) | ||
| if root != '' && root != '/' && root !in seen { | ||
| seen[root] = true | ||
| dirs << root |
There was a problem hiding this comment.
When a workspace folder is removed while one of its documents remains open, drop_index_under deletes the folder's entries, but the next query derives the same v.mod root from that open document here and recursively indexes the removed folder again. Consequently workspace symbols and other index-backed results continue including unopened files from a folder the client removed; retain only the open buffer or otherwise exclude explicitly removed roots from this derivation.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 50b8b02f16
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if rest.len >= 2 && rest[1] == `=` && (rest.len < 3 || rest[2] != `=`) | ||
| && rest[0] in [`+`, `-`, `*`, `/`, `%`, `&`, `|`, `^`] { |
There was a problem hiding this comment.
When V code mutates a variable with bits <<= 1 or bits >>= 1, this condition misses the assignment because rest[1] is another angle bracket rather than =; the occurrence therefore falls through as a read highlight. Fresh evidence beyond the earlier increment/decrement report is that these three-character mutation operators remain unhandled after the ++/-- fix.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f8d70eab4a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| mut uris := app.symbol_index.keys() | ||
| uris.sort() | ||
| for uri in uris { | ||
| occ := app.occurrences_for(uri) |
There was a problem hiding this comment.
In a multi-root workspace, this loop collects same-named occurrences from every indexed root before resolving any anchors. Consequently, 49 unrelated uses of a name in another root push an otherwise unique rename in the current project over reference_semantic_max_candidates, and the allow_lexical_fallback == false path refuses the rename entirely. Filter candidates to the source symbol's project/search scope before applying the cap.
Useful? React with 👍 / 👎.
| fn (app &App) index_is_complete() bool { | ||
| if app.index_incomplete_scopes.len > 0 { | ||
| return false | ||
| } | ||
| for uri, _ in app.index_skipped_uris { | ||
| if os.is_file(uri_to_path(uri)) { | ||
| return false |
There was a problem hiding this comment.
When multiple workspace roots are indexed, this global check makes an oversized or unreadable file in any unrelated root disable rename everywhere: reindex_uri records that URI as skipped, index_is_complete() returns false, and handle_rename rejects a rename even when the target project was indexed completely. Track completeness per scope and check only the project/module relevant to the requested symbol.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b99fc08cb8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| && rest[0] in [`+`, `-`, `*`, `/`, `%`, `&`, `|`, `^`] { | ||
| return doc_highlight_write | ||
| } | ||
| return doc_highlight_read |
There was a problem hiding this comment.
When the highlighted symbol is declared without an assignment operator, such as a function parameter (fn f(item Type)) or loop binding (for item in items), none of the preceding operator checks match and the declaration falls through as a read. This makes the new read/write highlighting mislabel declarations despite the classifier's stated behavior; recognize declaration contexts in addition to operators following the identifier.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8513d7a863
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if a := anchor { | ||
| if resolved := app.resolve_symbol_anchor_cached(uri, line_idx, start_char, mut | ||
| anchor_cache) |
There was a problem hiding this comment.
When the selected symbol has many occurrences in one document and its initial anchor resolves, this invokes resolve_symbol_anchor_cached for every occurrence; because each position has a distinct cache key, every lookup launches a separate run_v_line_info compiler process. An automatic document-highlight request for a common variable can therefore run dozens or hundreds of serial compilations, each with a 30-second timeout, while the synchronous request loop cannot service other messages; cap or batch these semantic checks as the reference scan does.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 139d054afc
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } | ||
| return app.resolve_symbol_anchor(uri, line, ch) | ||
| } |
There was a problem hiding this comment.
When a document contains more than 48 occurrences of the selected spelling, this returns none, causing the downstream semantic filter to be skipped entirely. For common names reused in multiple functions, highlighting one local variable then highlights every unrelated same-named variable in the document; the capped path should return no highlights or use a scope-safe resolver rather than treating all lexical matches as the same symbol.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 528f2e8b91
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| break | ||
| } | ||
| // Skip string literals. | ||
| if c == `"` || c == `'` { |
There was a problem hiding this comment.
When a symbol is referenced inside a V string interpolation, such as 'hello ${name}', this branch skips the entire literal and never records the reference. handle_rename builds its edits exclusively from these indexed candidates, so renaming name updates its declaration and ordinary uses but leaves the interpolation unchanged, producing a partial rename that can make the program fail to compile; scan interpolation expressions while continuing to ignore literal text.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4a7b91c8d9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| full := os.join_path(dir, entry) | ||
| if !os.is_file(full) { | ||
| continue | ||
| } | ||
| uri := index_uri_for_path(full, open_uris_by_path) |
There was a problem hiding this comment.
When a loose module contains a .v symlink whose target is outside the module directory, os.is_file(full) follows the link and this path indexes it without the canonical containment check used by the recursive walker. Workspace symbols can consequently expose declarations from outside the requested scope, and rename can return an edit addressed through the external symlink; resolve each shallow candidate and reject targets outside the canonical dir before indexing it.
Useful? React with 👍 / 👎.
| text := lines[line] | ||
| byte_col := utf8_char_to_byte_index(text, col) | ||
| byte_col := encoded_col_to_byte(text, col, app.position_encoding) |
There was a problem hiding this comment.
When an open document uses an equivalent noncanonical URI such as file://localhost/tmp/a.v or a UNC-authority spelling, get_word_at_position reconstructs a different URI from the filesystem path and therefore selects the on-disk text before performing this position conversion. With unsaved edits, references and rename can resolve the wrong symbol or return no result. Fresh evidence beyond the earlier URI fixes is that both handlers still pass only real_path into this helper; pass the original URI or resolve it through the normalized open-document map before reading the content.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f74ef58d66
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // server-initiated request (progress create, capability registration). | ||
| // Consume it without dispatching so it never hits the method router or | ||
| // produces a spurious MethodNotFound error (P0-02). | ||
| if is_client_response_message(content) { |
There was a problem hiding this comment.
This classifies the payload using the permissive member scanner before json2.decode validates the JSON, so malformed input such as {"id":2,"result":null garbage} is silently consumed instead of producing a parse error. If the ID matches the watched-files registration, note_server_request_response also marks watchers active and disables timed index refresh even though no valid acknowledgement was received; validate the complete response envelope before consuming or recording it.
Useful? React with 👍 / 👎.
| } | ||
| // The buffer is gone; re-index from disk so the file's symbols remain | ||
| // discoverable with their on-disk content. | ||
| app.reindex_uri(uri) |
There was a problem hiding this comment.
When a document opened as an equivalent noncanonical URI such as file://localhost/tmp/a.v is closed, this reindexes the disk file under that client URI after removing it from open_files. If an active watcher later reports the canonical file:///tmp/a.v, the open-path alias map is now empty, so the watcher creates a second index entry and both copies can appear in workspace symbols or reference candidates. Fresh evidence beyond the earlier open-buffer watcher issue is this newly added close-time reindex; normalize to the disk URI or reconcile all equivalent index keys here.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3a4708db6a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if col + 1 < line_text.len && c == `/` && line_text[col + 1] == `/` { | ||
| return line_text.len |
There was a problem hiding this comment.
When a source contains /* ... */ comments, this scanner recognizes only // and indexes every identifier inside the block as a reference candidate. If a block comment repeats the renamed symbol enough times to exceed reference_semantic_max_candidates, rename is refused; references above the cap instead return those comment words as locations. Track block-comment state across lines as the semantic-token scanner already does.
Useful? React with 👍 / 👎.
| break | ||
| } | ||
| // Skip string literals. | ||
| if c == `"` || c == `'` { |
There was a problem hiding this comment.
When a variable is used in a V interpolation such as 'hello ${name}' or '$name', this branch skips the entire string, including the executable interpolation expression. Document highlighting from either the declaration or the interpolation therefore omits that reference—and when invoked inside the interpolation can even omit the selected occurrence itself. Scan interpolation expressions while continuing to ignore ordinary literal text, as the reference index does.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 159880016d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| real_file := os.real_path(full).replace('\\', '/') | ||
| if path_is_within(real_file, canonical_root) { | ||
| acc << full |
There was a problem hiding this comment.
When a workspace contains both real.v and an in-tree alias.v symlink to it, containment passes for both but this appends their distinct lexical paths. ensure_dirs_indexed consequently creates two URI-keyed entries, so query_workspace_symbols returns every declaration twice because its deduplication key includes the URI; a semantic rename that accepts both candidates can also emit two edits against the same inode. Track canonical file targets in the visited set or otherwise deduplicate them before appending.
Useful? React with 👍 / 👎.
1 participant
This branch works through a static architecture/correctness/security audit of
the language server. It is a large but coherent body of work; commits are
grouped by theme below and each is self-contained (build stays green, all
_test.vsuites pass at every step).Protocol / JSON-RPC
verbatim; string ids no longer collide with numeric
0in cancellation).jsonmodule tojson2.Position encoding
utf32 support, negotiated at
initialize.re-encoded consistently with the negotiated encoding.
Security / process execution
Diagnostics
v -check -json-errors/-line-infowritetheir JSON to stderr, so returning stdout alone silently dropped every
diagnostic. Both streams are now merged.
definition/signature-help resolve real content (not just a response id) so
this class of bug cannot hide again.
Indexing / navigation
hover docs, call-hierarchy prepare, and module completions.
(validated against a compiler anchor), plus read/write document highlights.
symlink-cycle protection in the index walk.
Document sync & safe edits
advancing the document version.
didSavenever marks an unopened document open; never falls back to a globalbuffer for another document.
computed from byte offsets; keyword rename targets are rejected.
Performance
and re-reading vlib from disk on every request.
Testing
v .builds clean andv test .passes all six_test.vsuites. Testcoverage was expanded substantially (index, references, encoding, and
content-asserting integration tests for compiler-backed features).