Skip to content

Commit c59f81c

Browse files
committed
fix(lsp-server): handle unicode custom root diagnostics parity
1 parent d4500ec commit c59f81c

4 files changed

Lines changed: 285 additions & 8 deletions

File tree

AGENTS.md

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,3 +8,12 @@
88
- When adding a new runtime dependency to `packages/lsp-server/package.json`, also review `packages/lsp-server/tsup.config.ts`.
99
- If the dependency is imported by the server runtime code, add it to `nodeConfig.noExternal` unless there is a clear reason to keep it external.
1010
- Reason: `packages/lsp-server/scripts/prepare-client-assets.js` copies built server artifacts into `lsp-client`; required runtime dependencies should be bundled to avoid missing module errors at extension runtime.
11+
12+
## Playground Browser Testing Notes
13+
- When editing files inside the VSCode Web playground, prefer whole-file replacement (`focus editor -> Select All -> type/paste full content`) over partial line edits.
14+
- Reason: editor language features (auto-indent, snippet/format behaviors, newline handling) can introduce unintended indentation or syntax changes during incremental typing.
15+
- Always verify editor focus before typing. VSCode Web frequently shifts focus to the command palette, Problems panel, diff editors, or other UI panels.
16+
- Prefer minimizing the repro fixture (for example, reduce `main.bean` includes to only the files under test) to reduce noise and focus-switch count.
17+
- After browser automation edits, verify actual file contents before trusting diagnostics. Useful pattern: add/call debug commands that copy active file content or a project snapshot.
18+
- Be careful when using command palette automation: fuzzy search can select similarly named commands (for example compare-with-clipboard flows) and open diff editors unexpectedly.
19+
- If a syntax error appears immediately after automated typing, first suspect accidental auto-indent / misplaced whitespace from editor input rather than parser/runtime incompatibility.

docs/specs/beancount-lsp-p0-correctness-implementation-summary-2026-02-25.md

Lines changed: 94 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -258,6 +258,100 @@ Changes:
258258
- Reused shared config loading in both initial registration and config-changed path
259259
- Reworked `mergeAndDedupDiagnostics()` key to include:
260260
- start/end range
261+
262+
## G. Post-Implementation Follow-up (2026-02-26)
263+
264+
### G1. Root-name option changes now trigger diagnostics revalidation
265+
266+
Problem observed in playground regression:
267+
268+
- Editing `main.bean` to add/remove `option "name_assets"` (and related root-name options) updated effective options correctly, but existing diagnostics in other open files could remain stale until another trigger.
269+
270+
Fix implemented:
271+
272+
- `packages/lsp-server/src/common/features/diagnostics.ts`
273+
- Extended diagnostics revalidation trigger set (`REVALIDATE_ON_OPTION_CHANGE`) to include:
274+
- `name_assets`
275+
- `name_liabilities`
276+
- `name_equity`
277+
- `name_income`
278+
- `name_expenses`
279+
280+
Observed effect:
281+
282+
- Adding a custom root (e.g. `Actifs`) produces expected root-account diagnostics.
283+
- Removing/restoring the option clears those diagnostics automatically without stale leftovers.
284+
285+
### G2. Browser WASM custom non-ASCII root compat filter (beancheck parity workaround)
286+
287+
Problem observed:
288+
289+
- In browser WASM runtime, `beancheck` diagnostics could report `Invalid account name: <non-ASCII-root>:...` for custom non-ASCII root names, while local Python Beancount accepted equivalent minimal examples.
290+
- Tree-sitter parsing and local LSP account-root validation supported the Unicode roots, so this was not a parser/local-validation issue.
291+
292+
Fix implemented:
293+
294+
- `packages/lsp-server/src/common/features/diagnostics.ts`
295+
- Added a browser-WASM-only compatibility filter for beancheck diagnostics when all conditions hold:
296+
- runtime mode is `wasm`
297+
- custom root options are active
298+
- at least one custom root contains non-ASCII characters
299+
- Suppresses beancheck diagnostics matching custom non-ASCII roots for:
300+
- `Invalid account name: ...`
301+
- `Invalid reference to unknown account '...'` (chained noise)
302+
- Shows a one-time warning that browser WASM diagnostics were partially suppressed and local Python runtime is authoritative.
303+
304+
Observed effect in playground:
305+
306+
- Russian custom roots (`Активы / Пассивы / Капитал / Доходы / Расходы`) can be used in `open` directives and transactions without beancheck noise flooding the Problems panel (browser WASM mode).
307+
308+
### G3. Diagnostics source labeling and message language normalization
309+
310+
Problem observed:
311+
312+
- It was hard to distinguish which diagnostics came from local LSP checks vs runtime beancheck.
313+
- Root account local validation message was emitted in Chinese while surrounding diagnostics were otherwise English-first.
314+
315+
Fix implemented:
316+
317+
- `packages/lsp-server/src/common/features/diagnostics.ts`
318+
- Split `Diagnostic.source` values:
319+
- local diagnostics: `beancount-lsp (lsp)`
320+
- runtime beancheck diagnostics: `beancount-lsp (beancheck)`
321+
- Converted root-account validation message to English:
322+
- `Invalid root account name "...". Valid root account names: ...`
323+
324+
Reviewer note:
325+
326+
- This intentionally keeps the existing validation strategy (open-document local validation does not require inclusion from `main.bean`).
327+
328+
### G4. Playground debug commands for browser repro verification
329+
330+
Problem observed:
331+
332+
- VSCode Web focus/panel interactions can make browser automation appear successful while text was not actually written into the intended editor.
333+
334+
Implementation:
335+
336+
- `packages/playground/src/main.ts`
337+
- Added debug commands:
338+
- `demo.copyActiveFileContent`
339+
- `demo.copyAllProjectFilesSnapshot`
340+
341+
Use:
342+
343+
- Verify active editor content after each replacement (especially non-ASCII root tests).
344+
- Capture deterministic file snapshots (`path`, `length`, `sha256`, `head`) for repro handoff.
345+
346+
### G5. Browser validation findings (manual playground runs)
347+
348+
Validated behaviors in VSCode Web playground (WASM v3):
349+
350+
- Custom root options in Cyrillic are recognized by local validation and root lists.
351+
- Account names using Cyrillic custom roots parse and render in editor/UI (outline/breadcrumb/codelens paths observed).
352+
- `F2` rename can be invoked on Cyrillic account names (prepare-rename succeeds and pre-fills full Cyrillic account name).
353+
- Opening a file not included by `main.bean` can still produce local diagnostics:
354+
- this is expected because local LSP validation runs on open documents independently of beancheck include graph.
261355
- severity
262356
- code
263357
- message

packages/lsp-server/src/common/features/diagnostics.ts

Lines changed: 128 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -32,19 +32,108 @@ interface DiagnosticsConfig {
3232
warnOnIncompleteTransaction: boolean;
3333
}
3434

35+
const DIAGNOSTIC_SOURCE_LOCAL = 'beancount-lsp (lsp)';
36+
const DIAGNOSTIC_SOURCE_BEANCHECK = 'beancount-lsp (beancheck)';
37+
38+
const ROOT_OPTION_NAMES = [
39+
'name_assets',
40+
'name_liabilities',
41+
'name_equity',
42+
'name_income',
43+
'name_expenses',
44+
] as const;
45+
46+
const DEFAULT_ROOT_NAMES = new Set(['Assets', 'Liabilities', 'Equity', 'Income', 'Expenses']);
47+
const NON_ASCII_RE = /[^\x00-\x7F]/;
48+
const INVALID_ACCOUNT_NAME_RE = /^Invalid account name:\s*(.+)$/;
49+
const INVALID_UNKNOWN_ACCOUNT_RE = /^Invalid reference to unknown account ['"](.+?)['"]$/;
50+
3551
function isNotGitUri(uri: string): boolean {
3652
return URI.parse(uri).scheme !== 'git';
3753
}
3854

55+
function extractAccountFromBeancheckMessage(message: string): string | null {
56+
return message.match(INVALID_ACCOUNT_NAME_RE)?.[1]
57+
?? message.match(INVALID_UNKNOWN_ACCOUNT_RE)?.[1]
58+
?? null;
59+
}
60+
61+
function shouldSuppressCustomRootParityDiagnostic(
62+
diag: Diagnostic,
63+
suppressibleRoots: Set<string>,
64+
): boolean {
65+
const account = extractAccountFromBeancheckMessage(diag.message);
66+
if (!account) {
67+
return false;
68+
}
69+
const root = account.split(':')[0];
70+
return root != null && suppressibleRoots.has(root);
71+
}
72+
73+
interface BrowserCustomRootCompatFilterInput {
74+
runtimeMode: 'off' | 'local' | 'wasm';
75+
validRootAccounts: Set<string>;
76+
customNonAsciiRoots: Set<string>;
77+
}
78+
79+
interface BrowserCustomRootCompatFilterResult {
80+
diagnosticsByUri: Record<string, Diagnostic[]>;
81+
suppressedCount: number;
82+
}
83+
84+
function filterBeancheckDiagnosticsForBrowserCustomRootCompat(
85+
diagnosticsByUri: Record<string, Diagnostic[]>,
86+
input: BrowserCustomRootCompatFilterInput,
87+
): BrowserCustomRootCompatFilterResult {
88+
if (input.runtimeMode !== 'wasm' || input.customNonAsciiRoots.size === 0) {
89+
return { diagnosticsByUri, suppressedCount: 0 };
90+
}
91+
92+
const suppressibleRoots = new Set(
93+
[...input.customNonAsciiRoots].filter(root => input.validRootAccounts.has(root)),
94+
);
95+
if (suppressibleRoots.size === 0) {
96+
return { diagnosticsByUri, suppressedCount: 0 };
97+
}
98+
99+
let suppressedCount = 0;
100+
const next: Record<string, Diagnostic[]> = {};
101+
for (const [uri, diagnostics] of Object.entries(diagnosticsByUri)) {
102+
const kept = diagnostics.filter(diag => {
103+
if (!shouldSuppressCustomRootParityDiagnostic(diag, suppressibleRoots)) {
104+
return true;
105+
}
106+
suppressedCount += 1;
107+
return false;
108+
});
109+
if (kept.length > 0) {
110+
next[uri] = kept;
111+
}
112+
}
113+
114+
return { diagnosticsByUri: next, suppressedCount };
115+
}
116+
39117
export class DiagnosticsFeature implements Feature {
40118
private logger = new Logger('DiagnosticsFeature');
119+
private static readonly REVALIDATE_ON_OPTION_CHANGE = new Set([
120+
'infer_tolerance_from_cost',
121+
'inferred_tolerance_multiplier',
122+
'name_assets',
123+
'name_liabilities',
124+
'name_equity',
125+
'name_income',
126+
'name_expenses',
127+
]);
41128
private config: DiagnosticsConfig = {
42129
tolerance: 0.005, // Default tolerance
43130
warnOnIncompleteTransaction: true, // Default to show warnings for incomplete transactions
44131
};
45132
private diagnosticsFromBeancount: { [uri: string]: Diagnostic[] } = {};
46133
private standaloneBeancountDiagnosticUris = new Set<string>();
47134
private readonly validationTokenByUri = new Map<string, CancellationTokenSource>();
135+
private connection: Connection | undefined;
136+
private hasShownBrowserCustomRootParityWarning = false;
48137

49138
constructor(
50139
private readonly documents: DocumentStore,
@@ -55,6 +144,7 @@ export class DiagnosticsFeature implements Feature {
55144

56145
async register(connection: Connection): Promise<void> {
57146
this.logger.info('Registering diagnostics feature');
147+
this.connection = connection;
58148

59149
// Register callback on global bus to update diagnosticsFromBeancount on save
60150
const onBeancountDiagnosticsUpdated = async () => {
@@ -76,7 +166,7 @@ export class DiagnosticsFeature implements Feature {
76166
});
77167

78168
this.optionsManager.onOptionChange(async e => {
79-
if (['infer_tolerance_from_cost', 'inferred_tolerance_multiplier'].includes(e.name)) {
169+
if (DiagnosticsFeature.REVALIDATE_ON_OPTION_CHANGE.has(e.name)) {
80170
await this.validateAllDocuments(connection);
81171
}
82172
});
@@ -165,7 +255,7 @@ export class DiagnosticsFeature implements Feature {
165255
severity,
166256
range,
167257
message,
168-
source: 'beancount-lsp',
258+
source: DIAGNOSTIC_SOURCE_BEANCHECK,
169259
} as Diagnostic;
170260

171261
const uri = file.includes('://') ? file : `file://${file}`;
@@ -187,7 +277,37 @@ export class DiagnosticsFeature implements Feature {
187277

188278
addDiagnostic(DiagnosticSeverity.Warning, f.line, f.file, f.message);
189279
});
190-
this.diagnosticsFromBeancount = diagnosticsFromBeancount;
280+
const { diagnosticsByUri, suppressedCount } = filterBeancheckDiagnosticsForBrowserCustomRootCompat(
281+
diagnosticsFromBeancount,
282+
this.getBrowserCustomRootCompatFilterInput(),
283+
);
284+
this.diagnosticsFromBeancount = diagnosticsByUri;
285+
286+
if (suppressedCount > 0 && !this.hasShownBrowserCustomRootParityWarning) {
287+
this.hasShownBrowserCustomRootParityWarning = true;
288+
const message = 'Browser Beancount WASM runtime may not fully support custom non-ASCII root account names. Diagnostics were partially suppressed; switch to Local (Python) runtime for authoritative checks.';
289+
this.logger.warn(`${message} suppressed=${suppressedCount}`);
290+
void this.connection?.window.showWarningMessage(message);
291+
}
292+
}
293+
294+
private getBrowserCustomRootCompatFilterInput(): BrowserCustomRootCompatFilterInput {
295+
const runtimeMode = this.beanMgr?.getRuntimeStatus().mode ?? 'off';
296+
const validRootAccounts = this.optionsManager.getValidRootAccounts();
297+
const customNonAsciiRoots = new Set<string>();
298+
for (const name of ROOT_OPTION_NAMES) {
299+
const option = this.optionsManager.getOption(name);
300+
const value = option.asString();
301+
if (option.isDefault || !NON_ASCII_RE.test(value) || DEFAULT_ROOT_NAMES.has(value)) {
302+
continue;
303+
}
304+
customNonAsciiRoots.add(value);
305+
}
306+
return {
307+
runtimeMode,
308+
validRootAccounts,
309+
customNonAsciiRoots,
310+
};
191311
}
192312

193313
private async validateDocument(document: TextDocument, connection: Connection): Promise<void> {
@@ -274,7 +394,7 @@ export class DiagnosticsFeature implements Feature {
274394
severity: DiagnosticSeverity.Warning,
275395
range: transaction.headerRange,
276396
message: `transaction flagged with "!": ${document.getText(transaction.headerRange)}`,
277-
source: 'beancount-lsp',
397+
source: DIAGNOSTIC_SOURCE_LOCAL,
278398
});
279399
}
280400

@@ -314,7 +434,7 @@ export class DiagnosticsFeature implements Feature {
314434
severity: DiagnosticSeverity.Error,
315435
range: transaction.headerRange,
316436
message: `Transaction does not balance: ${imbalanceMessages.join(', ')}`,
317-
source: 'beancount-lsp',
437+
source: DIAGNOSTIC_SOURCE_LOCAL,
318438
});
319439
}
320440
}
@@ -348,7 +468,7 @@ export class DiagnosticsFeature implements Feature {
348468
end: { line, character: Math.max(0, lineText.length) },
349469
},
350470
message: 'Balance line incomplete: quick fix can complete current account balance',
351-
source: 'beancount-lsp',
471+
source: DIAGNOSTIC_SOURCE_LOCAL,
352472
code: 'balance-missing-amount',
353473
});
354474
}
@@ -627,8 +747,8 @@ export class DiagnosticsFeature implements Feature {
627747
diagnostics.push({
628748
severity: DiagnosticSeverity.Error,
629749
range: asLspRange(accountNode),
630-
message: `无效的根账户名称 "${root}"。有效的根账户名称: ${validRootsList}`,
631-
source: 'beancount-lsp',
750+
message: `Invalid root account name "${root}". Valid root account names: ${validRootsList}`,
751+
source: DIAGNOSTIC_SOURCE_LOCAL,
632752
code: 'invalid-root-account',
633753
});
634754
}

packages/playground/src/main.ts

Lines changed: 54 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -71,6 +71,27 @@ function debugLog(...args: unknown[]) {
7171
console.log('[playground]', ...args);
7272
}
7373

74+
function hasNonAscii(text: string): boolean {
75+
return /[^\x00-\x7F]/.test(text);
76+
}
77+
78+
async function sha256Hex(text: string): Promise<string> {
79+
const data = new TextEncoder().encode(text);
80+
const digest = await globalThis.crypto.subtle.digest('SHA-256', data);
81+
return Array.from(new Uint8Array(digest))
82+
.map(byte => byte.toString(16).padStart(2, '0'))
83+
.join('');
84+
}
85+
86+
async function copyTextWithFallback(text: string, promptTitle: string, successMessage: string): Promise<void> {
87+
try {
88+
await navigator.clipboard.writeText(text);
89+
void window.showInformationMessage(successMessage);
90+
} catch {
91+
globalThis.prompt(promptTitle, text);
92+
}
93+
}
94+
7495
// ---------------------------------------------------------------------------
7596
// File System Access API detection
7697
// ---------------------------------------------------------------------------
@@ -882,6 +903,39 @@ commands.registerCommand('demo.copyShareUrl', async () => {
882903
}
883904
});
884905

906+
commands.registerCommand('demo.copyActiveFileContent', async () => {
907+
const editor = window.activeTextEditor;
908+
if (!editor || !isProjectFile(editor.document.uri)) {
909+
void window.showInformationMessage('Open a project .bean file editor first.');
910+
return;
911+
}
912+
const text = editor.document.getText();
913+
await copyTextWithFallback(
914+
text,
915+
`Copy active file content (${editor.document.uri.path})`,
916+
`Copied active file content (${text.length} chars, non-ASCII: ${hasNonAscii(text) ? 'yes' : 'no'}).`,
917+
);
918+
});
919+
920+
commands.registerCommand('demo.copyAllProjectFilesSnapshot', async () => {
921+
const projectFiles = Array.from(stateFiles.entries())
922+
.filter(([path]) => path.startsWith(PROJECT_PATH_PREFIX))
923+
.sort(([a], [b]) => a.localeCompare(b));
924+
const snapshot = await Promise.all(projectFiles.map(async ([path, content]) => ({
925+
path,
926+
length: content.length,
927+
hasNonAscii: hasNonAscii(content),
928+
sha256: await sha256Hex(content),
929+
head: content.slice(0, 160),
930+
})));
931+
const text = JSON.stringify(snapshot, null, 2);
932+
await copyTextWithFallback(
933+
text,
934+
'Copy all project files snapshot',
935+
`Copied project snapshot (${snapshot.length} files).`,
936+
);
937+
});
938+
885939
commands.registerCommand('demo.resetDemo', () => {
886940
if (!isMemfsMode) {
887941
void window.showInformationMessage('Reset is only available in memfs mode.');

0 commit comments

Comments
 (0)