fix: refresh cached project sessions from disk and guard fix writes against stale content

[Arthurvdv]

Summary

• ProjectSession.RefreshFromDiskAsync incrementally syncs the in-memory workspace with on-disk .al files before every get_fixes / apply_fix / apply_fix_all call. Only changed/added/removed files are touched (length+mtime gate, then ordinal content compare); untouched documents keep their DocumentId and compilation state. Mutations go through Workspace.OnDocument* like almcp's ProjectWatcher.
• GuardedFileWriter.WriteIfUnchangedAsync verifies the file still equals the text the fix was computed from before writing. apply_fix returns { error: "StaleFile" } on conflict; apply_fix_all writes non-conflicting files, lists the rest in conflicts and keeps their diagnostics in unfixedDiagnostics.
• ReloadProjectAsync (full teardown + reload) is removed; the incremental refresh replaces it.
• Robustness from the review rounds: a locked, vanished or permission-denied file is skipped for that refresh (stderr Warning:) instead of failing the whole call; a failing write in apply_fix_all becomes a conflicts entry instead of aborting the loop; refresh bookkeeping is committed in a finally, so a cancelled refresh cannot leave the tracked-document map out of sync with the workspace; ProjectSession.Dispose is idempotent and waits (bounded) for an in-flight refresh.

Root cause

ProjectSessionManager cached a ProjectSession per project path for the life of the server. ProjectLoader.LoadProjectAsync eagerly read every .al file into in-memory TextLoaders at load time, and nothing re-read them except the ReloadProjectAsync that apply_fix/apply_fix_all called after their own write.

External edits made between calls were invisible to get_fixes, apply_fix, and apply_fix_all. apply_fix_all wrote whole-file content computed from stale text and silently reverted external edits.

The issue mentions al_downloadsymbols force:true as a workaround. That only recreates almcp's own workspace (CompilationService.ReloadWorkspace), never ours — the in-process ProjectSession was unaffected by almcp's reload, so the staleness persisted.

AA0248

The this.MaxStrLen false positives from issue #36 come from Microsoft's CodeCop Rule248AddThis, which excludes built-ins via TargetMethod.IsStatic (every listed built-in is isStatic: true in the compiler symbol tables); the syntactic AddThisCodeAction only fires where the analyzer reported. Those false positives most plausibly came from unresolved invocations (error symbols, non-static) in a stale or symbol-less in-process compilation — a downstream effect of this same staleness bug. No code change for AA0248 in this PR; the refresh fix should eliminate the root cause.

Behaviour changes

apply_fix

On conflict (file changed on disk after the fix was computed):

{
  "error": "StaleFile",
  "message": "<filePath> changed on disk after the fix was computed; nothing was written. Re-run get_fixes and apply_fix.",
  "filePath": "...",
  "diagnosticId": "..."
}

apply_fix_all

The Completed response now always includes conflicts (empty [] when none):

{
  "applied": true,
  "dryRun": false,
  "diagnosticId": "LC0020",
  "filesChanged": ["PageA.al"],
  "conflicts": [
    { "filePath": "PageB.al", "message": "PageB.al changed on disk after the fix was computed; not overwritten." }
  ],
  "message": "1 file(s) were skipped because they changed on disk while the fix was being computed; their diagnostics are included in unfixedDiagnostics; re-run apply_fix_all to fix them.",
  "unfixedDiagnostics": [
    { "filePath": "PageB.al", "line": 11, "column": 17 }
  ]
}

• Diagnostics of files skipped as conflicts are included in unfixedDiagnostics (deduplicated against the ones the fix-all pass itself could not fix), so callers that check unfixedDiagnostics.length == 0 correctly see them as unresolved. Their positions are the analysis-time positions and may have shifted if the file was edited; the tool description says so.
• applied is true as soon as at least one file was written; conflicts and message carry the partial-success detail.
• A file whose write fails with an I/O or permission error is reported as a conflicts entry too; the remaining files are still processed.

No post-write reload

There is no explicit ReloadProjectAsync after writing. The next GetOrLoadProjectAsync call picks the written file up via the same RefreshFromDiskAsync that runs on every cache hit.

Tests

#	Test	Asserts
1	NothingChanged_SummaryAllZero_SameSolution	Refresh with no disk changes: summary all zero, same Solution instance, same DocumentIds
2	EditFile_UpdatedCountIsOne_NewTextVisible	Append // edited to PageB.al: Updated == 1, new text in GetDocument, IDs preserved
3	AddFile_AddedCountIsOne_DocumentAccessible	Create PageD.al: Added == 1, document accessible, doc count increased
4	DeleteFile_RemovedCountIsOne_CompilationSucceeds	Delete PageC.al: Removed == 1, document null, compilation succeeds
5	TouchWithoutContentChange_SummaryZero_SameSolution	Touch mtime only: summary zero, same Solution (content-compare fallback)
6	TouchWithoutContentChange_StampConverges	After a touch-only refresh the tracked stamp equals the file's stamp, so the next refresh skips the read
7	FileInAlPackages_NotAdded	Create .alpackages/X.al: Added == 0
8	ViaSessionManager_EditVisibleOnSecondCall_SameInstance	Edit between two GetOrLoadProjectAsync calls: same ProjectSession instance, edit visible
9	LockedFile_RefreshCompletes_FileSkipped	Windows-only: PageB.al held with an exclusive lock and touched; refresh completes with zero summary, no exception
10	PreCancelledToken_ThrowsBeforeMutating_NextRefreshPicksUpEverything	Already-cancelled token throws before any mutation; the next refresh adds both new files with no duplicates
11	Dispose_Idempotent	Calling Dispose twice does not throw
12	TryStat_NonexistentPath_ReturnsNull / TryReadAsync_NonexistentPath_ReturnsNull	The per-file guards return null instead of throwing
13	ApplyFixAll_ExternalEditBetweenCalls_PreservesExternalEdit	THE #36 regression test: prime session, rename OtherField to RenamedField in PageB.al, run apply_fix_all LC0020. Asserts PageB still contains RenamedField AND has exactly one ApplicationArea.
14	ApplyFixAll_FileAddedAfterLoad_FixesNewFile	Add PageD.al after load: diagnosticsFound == 3, PageD in filesChanged
15	ApplyFixAll_FileDeletedAfterLoad_FixesRemainingFiles	Delete PageB.al after load: diagnosticsFound == 1, only PageA in filesChanged
16	Existing ApplyFixAll_ProjectScope_FixesAllOccurrencesAcrossFiles	Added Assert.Empty(conflicts)
17	ApplyFixAll_Changes_CarryDiagnosticLocations	CodeFixRunner-level: each FixAllFileChange has non-empty Diagnostics with correct 1-based line numbers
18	MergeUnfixed_* (4 cases)	No conflicts returns the same list; a conflict appends its file's diagnostics; a diagnostic already unfixed is listed once; path match is case-insensitive
19	ApplyFix_ExternalEditBetweenCalls_PreservesEditAndAppliesFix	Prime, append // edited, apply fix: applied == true, file contains // edited, exactly one ApplicationArea
20	GuardedFileWriterTests (4 cases)	Matching file writes; differing file returns a conflict and stays untouched; missing file and missing parent directory return a conflict
21	EnumerateAlFiles_ExcludesAlPackages_ReturnsFullPaths	.alpackages/*.al excluded, full paths returned

Regression test vs old code: The new tests cannot compile against the old code (CodeFixResult gained OriginalContent, the JSON response gained conflicts), so the stash-and-run dance is impractical. From the diff: the old ApplyFixAllTool called File.WriteAllTextAsync(change.FilePath, change.ModifiedContent) with ModifiedContent computed from the cached (stale) workspace text, so test 13's RenamedField rename would have been overwritten by the stale OtherField content. The new code calls RefreshFromDiskAsync before computing the fix and uses GuardedFileWriter for the write, making the rename survive.

Verification

• dotnet build --configuration Release: 0 warnings, 0 errors. dotnet test --configuration Release: 257 passed, 0 skipped (Windows, DevTools 18.0.41).
• CI did not run on this PR: the GitHub Actions runner quota was exhausted on 2026-09-22. The Ubuntu leg of the matrix has therefore not exercised the new tests; the only OS-gated test (LockedFile_RefreshCompletes_FileSkipped) returns early off Windows.
• Four high-effort review rounds; every confirmed finding was fixed in the fix: address review round N commits. Follow-up: ProjectSession.Dispose disposes the workspace while a timed-out refresh may still be running #38 (Dispose still disposes the workspace when its bounded wait for an in-flight refresh times out; shutdown-only).

Fixes #36

🤖 Generated with Claude Code