kfickel/streamd

Fork 0

LSP timesheet diagnostics: doubled error prefix, duplicate publish, and missing cross-file timesheets #136

New issue

Closed

opened 2026-08-19 21:00:35 +02:00 by kfickel · 2 comments

kfickel commented

2026-08-19 21:00:35 +02:00

Owner

Found while inspecting an LSP session log. Three related issues in textDocument/diagnostic handling for R18 timesheet checks (src/cli/commands/lsp.rs):

1. Doubled "Timesheet error:" prefix

StreamdError::TimesheetError already formats as "Timesheet error: {0}" (src/error.rs:8). compute_diagnostics in src/cli/commands/lsp.rs:524 wraps the already-formatted error again:

message: format!("Timesheet error: {}", e),

Result: "Timesheet error: Timesheet error: Last Timecard of 2025-11-03 is not a break!".

Fix: use e.to_string() (or format!("{}", e)) without the extra prefix.

2. Diagnostics published twice, in inconsistent order

did_open (src/cli/commands/lsp.rs:728) unconditionally computes diagnostics and pushes them via publish_diagnostics. The server also implements pull diagnostics (textDocument/diagnostic, handled around lsp.rs:879), which independently recomputes and returns the same diagnostics in the RPC response. A client that opens a file and then pulls diagnostics gets the same diagnostic twice (once as a publishDiagnostics notification, once as the request response), and the relative ordering between the two is not deterministic — observed as response-then-push for one file and push-then-response for another in the same session, indicating a race rather than a defined sequence.

Should settle on one diagnostic delivery model per client capabilities (respect textDocument.diagnostic client capability to decide whether to also push on didOpen/didChange), or at minimum keep the two paths from stepping on each other.

3. Timesheet validation in the LSP is file-scoped, but timesheets are not

compute_diagnostics only extracts timesheets from the single open file's shard:

extract_timesheets(std::slice::from_ref(root), now, self.tz)

But timesheets are a repository-wide concept — src/cli/commands/timesheet.rs:328 builds the real report from all markdown shards in the base folder (load_markdown_shards, walked non-recursively), and extract_timesheets groups timecards by date across files. A day's entries can legitimately span more than one file (e.g. a stream file plus a note file dated the same day), so checking only the currently-open file against extract_timesheets can both:

false-positive: flag "Last Timecard of DATE is not a break!" or overlap errors when the missing/overlapping entry actually lives in a sibling file for the same date, and
false-negative: miss overlaps/violations that only become visible once other files for that date are included.

Fix: LSP diagnostics for R18 should load and merge shards for the whole base folder (like load_markdown_shards does for the CLI timesheet command) rather than just the single open file, then filter/report diagnostics for the open document's date(s). Needs some thought on caching (avoid re-walking + re-parsing the whole folder on every keystroke) — likely reuse/extend the existing file_cache (DashMap) to hold all repo files, not just open ones, and recompute the merged timesheet set from that cache.

Suggested scope for this ticket

Fix #1 (trivial, one-line).
Fix #2 (respect client diagnostic capabilities / avoid redundant push).
Fix #3 (extend LSP state to build timesheets from all repo shards, not just the open file).

Found while inspecting an LSP session log. Three related issues in `textDocument/diagnostic` handling for R18 timesheet checks (`src/cli/commands/lsp.rs`): ## 1. Doubled "Timesheet error:" prefix `StreamdError::TimesheetError` already formats as `"Timesheet error: {0}"` (`src/error.rs:8`). `compute_diagnostics` in `src/cli/commands/lsp.rs:524` wraps the already-formatted error again: ```rust message: format!("Timesheet error: {}", e), ``` Result: `"Timesheet error: Timesheet error: Last Timecard of 2025-11-03 is not a break!"`. Fix: use `e.to_string()` (or `format!("{}", e)`) without the extra prefix. ## 2. Diagnostics published twice, in inconsistent order `did_open` (`src/cli/commands/lsp.rs:728`) unconditionally computes diagnostics and pushes them via `publish_diagnostics`. The server also implements pull diagnostics (`textDocument/diagnostic`, handled around `lsp.rs:879`), which independently recomputes and returns the same diagnostics in the RPC response. A client that opens a file and then pulls diagnostics gets the same diagnostic twice (once as a `publishDiagnostics` notification, once as the request response), and the relative ordering between the two is not deterministic — observed as response-then-push for one file and push-then-response for another in the same session, indicating a race rather than a defined sequence. Should settle on one diagnostic delivery model per client capabilities (respect `textDocument.diagnostic` client capability to decide whether to also push on `didOpen`/`didChange`), or at minimum keep the two paths from stepping on each other. ## 3. Timesheet validation in the LSP is file-scoped, but timesheets are not `compute_diagnostics` only extracts timesheets from the single open file's shard: ```rust extract_timesheets(std::slice::from_ref(root), now, self.tz) ``` But timesheets are a repository-wide concept — `src/cli/commands/timesheet.rs:328` builds the real report from **all** markdown shards in the base folder (`load_markdown_shards`, walked non-recursively), and `extract_timesheets` groups timecards by date across files. A day's entries can legitimately span more than one file (e.g. a stream file plus a note file dated the same day), so checking only the currently-open file against `extract_timesheets` can both: - **false-positive**: flag "Last Timecard of DATE is not a break!" or overlap errors when the missing/overlapping entry actually lives in a sibling file for the same date, and - **false-negative**: miss overlaps/violations that only become visible once other files for that date are included. Fix: LSP diagnostics for R18 should load and merge shards for the whole base folder (like `load_markdown_shards` does for the CLI `timesheet` command) rather than just the single open file, then filter/report diagnostics for the open document's date(s). Needs some thought on caching (avoid re-walking + re-parsing the whole folder on every keystroke) — likely reuse/extend the existing `file_cache` (`DashMap`) to hold all repo files, not just open ones, and recompute the merged timesheet set from that cache. ## Suggested scope for this ticket - Fix #1 (trivial, one-line). - Fix #2 (respect client diagnostic capabilities / avoid redundant push). - Fix #3 (extend LSP state to build timesheets from all repo shards, not just the open file).

kfickel commented

2026-08-19 21:02:23 +02:00

Author

Owner

Implementation plan

1. Doubled "Timesheet error:" prefix — trivial

src/cli/commands/lsp.rs:524: change

message: format!("Timesheet error: {}", e),

message: e.to_string(),

StreamdError's Display (via thiserror, src/error.rs:8) already renders the full "Timesheet error: ..." text.

2. Stop double-publishing diagnostics

Root cause: the server always pushes (publish_diagnostics) from did_open/did_change/did_save and answers pull requests (textDocument/diagnostic), regardless of what the client asked for.

In initialize (lsp.rs:577), read params.capabilities.text_document.as_ref().and_then(|td| td.diagnostic.as_ref()).is_some() and store it as supports_pull_diagnostics: bool on LspState (set once in Backend::build_state, alongside config/tz/base_folder).
In did_open, did_change, did_save (lsp.rs:728, 740, 754): only call self.client.publish_diagnostics(...) when !state.supports_pull_diagnostics. Still always call state.parse_and_cache so the cache is warm for a subsequent pull.
Bump diagnostic_provider's inter_file_dependencies from false to true (lsp.rs:631) once #3 lands — diagnostics in one file can now depend on another file's content, which is exactly what that flag communicates to the client.
This is behavior clients already expect: a client that declared textDocument.diagnostic support is explicitly opting into pull mode, so unsolicited pushes are redundant per spec, not just noisy in this server.

3. Cross-file timesheets in the LSP

Today compute_diagnostics (lsp.rs:471) only feeds the single open file's cached shard into extract_timesheets. Timesheets are grouped by date across the whole repo (see timesheet.rs:328's load_markdown_shards + extract_timesheets(&all_shards, ...) for the CLI's streamd timesheet command), so a day split across two files can produce false positives/negatives when checked one file at a time.

Approach — reuse the cross-file iteration pattern already used by workspace_diagnostic/symbol/todo (lsp.rs:937, 992, 1048), which walks list_markdown_files(&state.base_folder) and lazily fills file_cache via parse_file_from_disk:

Add LspState::all_shards(&self) -> Vec<LocalizedShard>: iterate list_markdown_files(&self.base_folder), resolve each to a Url, get-or-populate file_cache (same lazy pattern as the existing loops), and collect the Some(root) entries.
Change the R18 block in compute_diagnostics to call extract_timesheets(&self.all_shards(), now, self.tz) instead of extract_timesheets(std::slice::from_ref(root), ...).
Attribution problem: Timesheet/TimesheetPoint (src/timesheet/extract.rs) carry no file origin, so once merged we can't tell which file "caused" a given date's violation. Fix by filtering after the fact: for the open uri, get that file's own shard-local dates via find_shard_by_set_dimension(std::slice::from_ref(root), TIMESHEET_DIMENSION_NAME) (already pub, already imported in lsp.rs) mapped through .moment.with_timezone(self.tz).date_naive() into a HashSet<NaiveDate>. Only emit a timesheet diagnostic (overlap or error) for ts.date if that date is in the open file's own date set. This means a violation shows up on every file that has an entry on the offending date (reasonable — the editor for any of those files should see it), but not on unrelated files.
workspace_diagnostic (lsp.rs:921) currently only reports todos; optionally extend it the same way so workspace/diagnostic pulls also surface R18 violations attributed per-file using the same date-membership filter — worth doing in this ticket since the plumbing is identical, but can be split into a follow-up if it makes the diff too large.
Caching cost: this makes every keystroke's did_change diagnostic computation walk+cache the whole base folder instead of one file. file_cache already avoids re-parsing unchanged files (only invalidated via did_change_watched_files, lsp.rs:719, and on open/change of the file itself), so steady-state cost is one directory walk (list_markdown_files) plus a DashMap lookup per file — acceptable for typical stream-note folder sizes, no further caching needed for this ticket.

Testing

Unit tests in lsp.rs's existing mod tests (lsp.rs:1153), following the tempdir-based pattern already used for list_markdown_files tests (lsp.rs:1320 etc.): create 2+ .md files in a tempdir whose combined @Timesheet markers span midday without a closing break only when merged, assert compute_diagnostics on file A (which alone looks fine) now reports the error once file B exists, and that neither file reports it once B supplies the closing break.
A test asserting the doubled-prefix message is gone (exact string match on the formatted diagnostic).
A test（or manual note in PR description) confirming publish_diagnostics is skipped when supports_pull_diagnostics is true, and still fires when a client omits that capability (back-compat for push-only clients).

Scope check

All three fixes stay within src/cli/commands/lsp.rs (plus reading the already-pub TIMESHEET_DIMENSION_NAME/find_shard_by_set_dimension); no changes needed to src/timesheet/ or src/error.rs. REQUIREMENTS.md/README.md should get a short note under R18 that timesheet diagnostics are repository-wide, per project convention of keeping the spec in sync with behavior.

## Implementation plan ### 1. Doubled "Timesheet error:" prefix — trivial `src/cli/commands/lsp.rs:524`: change ```rust message: format!("Timesheet error: {}", e), ``` to ```rust message: e.to_string(), ``` `StreamdError`'s `Display` (via `thiserror`, `src/error.rs:8`) already renders the full `"Timesheet error: ..."` text. ### 2. Stop double-publishing diagnostics Root cause: the server always pushes (`publish_diagnostics`) from `did_open`/`did_change`/`did_save` *and* answers pull requests (`textDocument/diagnostic`), regardless of what the client asked for. - In `initialize` (`lsp.rs:577`), read `params.capabilities.text_document.as_ref().and_then(|td| td.diagnostic.as_ref()).is_some()` and store it as `supports_pull_diagnostics: bool` on `LspState` (set once in `Backend::build_state`, alongside `config`/`tz`/`base_folder`). - In `did_open`, `did_change`, `did_save` (`lsp.rs:728`, `740`, `754`): only call `self.client.publish_diagnostics(...)` when `!state.supports_pull_diagnostics`. Still always call `state.parse_and_cache` so the cache is warm for a subsequent pull. - Bump `diagnostic_provider`'s `inter_file_dependencies` from `false` to `true` (`lsp.rs:631`) once #3 lands — diagnostics in one file can now depend on another file's content, which is exactly what that flag communicates to the client. - This is behavior clients already expect: a client that declared `textDocument.diagnostic` support is explicitly opting into pull mode, so unsolicited pushes are redundant per spec, not just noisy in this server. ### 3. Cross-file timesheets in the LSP Today `compute_diagnostics` (`lsp.rs:471`) only feeds the single open file's cached shard into `extract_timesheets`. Timesheets are grouped by *date* across the whole repo (see `timesheet.rs:328`'s `load_markdown_shards` + `extract_timesheets(&all_shards, ...)` for the CLI's `streamd timesheet` command), so a day split across two files can produce false positives/negatives when checked one file at a time. Approach — reuse the cross-file iteration pattern already used by `workspace_diagnostic`/`symbol`/`todo` (`lsp.rs:937`, `992`, `1048`), which walks `list_markdown_files(&state.base_folder)` and lazily fills `file_cache` via `parse_file_from_disk`: - Add `LspState::all_shards(&self) -> Vec<LocalizedShard>`: iterate `list_markdown_files(&self.base_folder)`, resolve each to a `Url`, get-or-populate `file_cache` (same lazy pattern as the existing loops), and collect the `Some(root)` entries. - Change the R18 block in `compute_diagnostics` to call `extract_timesheets(&self.all_shards(), now, self.tz)` instead of `extract_timesheets(std::slice::from_ref(root), ...)`. - **Attribution problem**: `Timesheet`/`TimesheetPoint` (`src/timesheet/extract.rs`) carry no file origin, so once merged we can't tell which file "caused" a given date's violation. Fix by filtering after the fact: for the open `uri`, get *that file's own* shard-local dates via `find_shard_by_set_dimension(std::slice::from_ref(root), TIMESHEET_DIMENSION_NAME)` (already `pub`, already imported in `lsp.rs`) mapped through `.moment.with_timezone(self.tz).date_naive()` into a `HashSet<NaiveDate>`. Only emit a timesheet diagnostic (overlap or error) for `ts.date` if that date is in the open file's own date set. This means a violation shows up on every file that has an entry on the offending date (reasonable — the editor for any of those files should see it), but not on unrelated files. - `workspace_diagnostic` (`lsp.rs:921`) currently only reports todos; optionally extend it the same way so `workspace/diagnostic` pulls also surface R18 violations attributed per-file using the same date-membership filter — worth doing in this ticket since the plumbing is identical, but can be split into a follow-up if it makes the diff too large. - Caching cost: this makes every keystroke's `did_change` diagnostic computation walk+cache the whole base folder instead of one file. `file_cache` already avoids re-parsing unchanged files (only invalidated via `did_change_watched_files`, `lsp.rs:719`, and on open/change of the file itself), so steady-state cost is one directory walk (`list_markdown_files`) plus a `DashMap` lookup per file — acceptable for typical stream-note folder sizes, no further caching needed for this ticket. ### Testing - Unit tests in `lsp.rs`'s existing `mod tests` (`lsp.rs:1153`), following the `tempdir`-based pattern already used for `list_markdown_files` tests (`lsp.rs:1320` etc.): create 2+ `.md` files in a tempdir whose combined `@Timesheet` markers span midday without a closing break only when merged, assert `compute_diagnostics` on file A (which alone looks fine) now reports the error once file B exists, and that neither file reports it once B supplies the closing break. - A test asserting the doubled-prefix message is gone (exact string match on the formatted diagnostic). - A test（or manual note in PR description) confirming `publish_diagnostics` is skipped when `supports_pull_diagnostics` is true, and still fires when a client omits that capability (back-compat for push-only clients). ### Scope check All three fixes stay within `src/cli/commands/lsp.rs` (plus reading the already-`pub` `TIMESHEET_DIMENSION_NAME`/`find_shard_by_set_dimension`); no changes needed to `src/timesheet/` or `src/error.rs`. `REQUIREMENTS.md`/`README.md` should get a short note under R18 that timesheet diagnostics are repository-wide, per project convention of keeping the spec in sync with behavior.

kfickel added the

planned

label

2026-08-19 21:02:28 +02:00

kfickel referenced this issue from a pull request that will close it,

2026-08-19 21:39:12 +02:00

Fix LSP timesheet diagnostics: doubled prefix, duplicate publish, cross-file dates #137

kfickel commented

2026-08-19 21:39:22 +02:00

Author

Owner

Implemented in PR #137 (branch 136_lsp-timesheet-diagnostics-fixes).

Summary of findings:

The three fixes from the plan comment landed roughly as scoped: doubled error prefix (one-liner), duplicate push/pull diagnostic delivery (gated by client textDocument.diagnostic capability), and cross-file timesheet merging (pre-filtered to files sharing a date with the open document, so an unrelated file's TimesheetError — which aborts the whole extract_timesheets call — can never leak onto this file's diagnostics).
While writing tests for the cross-file merge, a related but separate bug turned up during interactive debugging with the ticket reporter: an unclosed @Timesheet card logged for later today (before that time arrives) produced an inverted timecard (from 12:00 to 11:00) because the "close at now" synthetic-close fallback in aggregate_timecard_day never checked that now was actually after the card's start time. Fixed by clamping to a zero-duration timecard, per the reporter's preference. This was bundled into the same branch/PR rather than a separate ticket.
REQUIREMENTS.md (R18, R25b) updated to document both the cross-file validation behavior and the today-only synthetic-close clamping.
Not precisely tracked: wall-clock time and token usage for this session (interleaved with live debugging/clarification with the reporter, so not a clean single measurement). All work landed in 3 commits, 211 passing tests, clean nix flake check.

Implemented in PR #137 (branch `136_lsp-timesheet-diagnostics-fixes`). **Summary of findings:** - The three fixes from the plan comment landed roughly as scoped: doubled error prefix (one-liner), duplicate push/pull diagnostic delivery (gated by client `textDocument.diagnostic` capability), and cross-file timesheet merging (pre-filtered to files sharing a date with the open document, so an unrelated file's `TimesheetError` — which aborts the whole `extract_timesheets` call — can never leak onto this file's diagnostics). - While writing tests for the cross-file merge, a related but separate bug turned up during interactive debugging with the ticket reporter: an unclosed `@Timesheet` card logged for later today (before that time arrives) produced an **inverted timecard** (`from 12:00 to 11:00`) because the "close at now" synthetic-close fallback in `aggregate_timecard_day` never checked that `now` was actually after the card's start time. Fixed by clamping to a zero-duration timecard, per the reporter's preference. This was bundled into the same branch/PR rather than a separate ticket. - `REQUIREMENTS.md` (R18, R25b) updated to document both the cross-file validation behavior and the today-only synthetic-close clamping. - Not precisely tracked: wall-clock time and token usage for this session (interleaved with live debugging/clarification with the reporter, so not a clean single measurement). All work landed in 3 commits, 211 passing tests, clean `nix flake check`.

kfickel closed this issue

2026-08-19 21:50:01 +02:00

No labels

planned

No milestone

No project

No assignees

1 participant

Notifications

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Reference

kfickel/streamd#136

No description provided.

Rows
Columns