fix(themes): tighten editing-buffer save path (tmp cleanup, log-PII, line-refs)

Three review-pass fixes on SaveEditingBuffer:

- Wrap File.Move in try/catch that deletes the .tmp sibling on failure
  (AV-scanner lock, EXDEV, share-violation) then rethrows so the outer
  IOException catch still owns the error path. Avoids accumulating
  '<slug>.json.tmp' litter in the themes dir on retry storms.

- Reduce PII in the five new LogWarning calls that previously included
  full paths containing the user's home directory. Filename-only via
  Path.GetFileName is sufficient for triage; the two forensics-critical
  path-escape log calls keep full paths because diagnosing the escape
  needs the resolved target. WHY-comment anchors the v1.8.0 PII
  re-audit roadmap.

- Replace seven hardcoded 'ThemeRegistry.cs:<line>' references in
  comments with method-name + symbol descriptions so future Switch/
  RefreshCustomCache refactors do not bit-rot the comments.

Build 0/0, csharpier clean.
This commit is contained in:
2026-05-26 13:16:57 +02:00
parent 4f81cd1f24
commit 11eb7b9e90
+75 -38
View File
@@ -116,8 +116,8 @@ public sealed class ThemeRegistry
// True try-pattern lookup: returns false when neither built-in nor custom // True try-pattern lookup: returns false when neither built-in nor custom
// cache holds the slug, no fallback to default. M3 ThemePicker uses this // cache holds the slug, no fallback to default. M3 ThemePicker uses this
// for card-rendering, M6 ThemeImportExportRow for fork-slug collisions. // for card-rendering, M6 ThemeImportExportRow for fork-slug collisions.
// Cold-cache fallback: LoadCustomBySlug only reverse-iterates the // Cold-cache fallback: see `LoadCustomBySlug` lookup-by-slug reverse
// pre-populated _customCache (see ThemeRegistry.cs:263-280). If a freshly // iteration — it only walks the pre-populated _customCache. If a freshly
// imported file has not been enumerated yet (or no warm-up ran), the first // imported file has not been enumerated yet (or no warm-up ran), the first
// lookup would miss silently. Drain RefreshCustomCache once on miss so the // lookup would miss silently. Drain RefreshCustomCache once on miss so the
// custom file gets picked up before the second lookup. // custom file gets picked up before the second lookup.
@@ -279,12 +279,12 @@ public sealed class ThemeRegistry
// CALLER CONTRACT: the buffer slug must NOT collide with a built-in slug. // CALLER CONTRACT: the buffer slug must NOT collide with a built-in slug.
// Switch() prefers built-ins over custom themes with the same slug // Switch() prefers built-ins over custom themes with the same slug
// (ThemeRegistry.cs:107-112), so saving a custom file under a built-in // (see `Switch` built-in-first lookup), so saving a custom file under
// slug persists the file but leaves the built-in active — looks green, // a built-in slug persists the file but leaves the built-in active —
// behaves broken. M4 ColorPicker DrawIdleState forks built-in themes // looks green, behaves broken. M4 ColorPicker DrawIdleState forks
// into a custom slug before BeginEditing, M6 ImportFromPath renames // built-in themes into a custom slug before BeginEditing, M6
// built-in-colliding imports to <slug>_imported. New call-sites must // ImportFromPath renames built-in-colliding imports to <slug>_imported.
// either fork first or rename to a non-built-in slug. // New call-sites must either fork first or rename to a non-built-in slug.
public bool SaveEditingBuffer(out string targetPath) public bool SaveEditingBuffer(out string targetPath)
{ {
targetPath = string.Empty; targetPath = string.Empty;
@@ -310,12 +310,12 @@ public sealed class ThemeRegistry
// Safe-by-construction: refuse any slug that collides with a built-in // Safe-by-construction: refuse any slug that collides with a built-in
// BEFORE we touch the disk. Switch() prefers built-ins over custom files // BEFORE we touch the disk. Switch() prefers built-ins over custom files
// with the same slug (ThemeRegistry.cs:107-112). Without this reject a // with the same slug (see `Switch` built-in-first lookup). Without this
// mis-routed caller (or a future bug in ImportFromPath) could persist a // reject a mis-routed caller (or a future bug in ImportFromPath) could
// custom file under a built-in slug — the file lands on disk, Switch // persist a custom file under a built-in slug — the file lands on disk,
// keeps the built-in active, and the post-save active-slug check below // Switch keeps the built-in active, and the post-save active-slug check
// returns false. The caller then sees "save failed" while a garbage file // below returns false. The caller then sees "save failed" while a garbage
// accumulates in the themes dir on every retry. M4 ColorPicker forks // file accumulates in the themes dir on every retry. M4 ColorPicker forks
// built-in themes into a custom slug before BeginEditing, M6 ImportFromPath // built-in themes into a custom slug before BeginEditing, M6 ImportFromPath
// renames built-in-colliding imports to <slug>_imported, so production // renames built-in-colliding imports to <slug>_imported, so production
// paths already steer clear; this guard catches everything else. // paths already steer clear; this guard catches everything else.
@@ -361,20 +361,41 @@ public sealed class ThemeRegistry
// mid-write crash (power loss, Wine kill, OOM) leaves either the // mid-write crash (power loss, Wine kill, OOM) leaves either the
// previous content or the new content on disk, never a partial JSON // previous content or the new content on disk, never a partial JSON
// that would silently disappear at next Plugin-Start through the // that would silently disappear at next Plugin-Start through the
// ThemeJsonLoader catch-and-continue path (ThemeRegistry.cs:319-322). // ThemeJsonLoader catch-and-continue path inside RefreshCustomCache.
var tmpPath = targetPath + ".tmp"; var tmpPath = targetPath + ".tmp";
File.WriteAllText(tmpPath, json); File.WriteAllText(tmpPath, json);
File.Move(tmpPath, targetPath, overwrite: true); try
{
File.Move(tmpPath, targetPath, overwrite: true);
}
catch
{
// Avoid `.tmp` litter when Move fails (target locked by AV
// scanner, EXDEV cross-device, share-violation). Best-effort
// delete, then rethrow so the outer IOException catch still
// reports the failure.
try
{
File.Delete(tmpPath);
}
catch
{
// best-effort cleanup
}
throw;
}
// Note: the redundant `_lastActiveStamp = DateTime.MinValue` reset from // Note: the redundant `_lastActiveStamp = DateTime.MinValue` reset from
// the earlier plan-draft was removed — Switch() itself already resets // the earlier plan-draft was removed — Switch() itself already resets
// _lastActiveStamp on the custom-theme path (ThemeRegistry.cs:124) as // _lastActiveStamp on the custom-theme path (see `Switch`
// part of the active-switch, so a pre-Switch reset is overwritten anyway. // custom-theme branch resets `_lastActiveStamp`) as part of the
// active-switch, so a pre-Switch reset is overwritten anyway.
// RefreshCustomCache is a yield-iterator (ThemeRegistry.cs:282) — a bare // `RefreshCustomCache` is a yield-iterator (see its `yield return`
// call would build the iterator but never enumerate it, so the cache // body) — a bare call would build the iterator but never enumerate
// side-effect (_customCache[key] = (theme, stamp)) would never run. // it, so the cache side-effect (_customCache[key] = (theme, stamp))
// Force-enumerate so the subsequent Switch() finds the freshly saved file. // would never run. Force-enumerate so the subsequent Switch() finds
// the freshly saved file.
foreach (var _ in RefreshCustomCache()) { } foreach (var _ in RefreshCustomCache()) { }
// Use the sanitised slug for Switch() too — the buffer's raw Slug // Use the sanitised slug for Switch() too — the buffer's raw Slug
@@ -391,12 +412,13 @@ public sealed class ThemeRegistry
Switch(targetSlug); Switch(targetSlug);
// Same-slug in-place edit: Switch() hits the Same-Slug-Noop-Return // Same-slug in-place edit: Switch() hits its same-slug noop
// (ThemeRegistry.cs:102-103) and leaves _active pointing at the // early-return (see `Switch` same-slug noop early-return) and
// PRE-edit Theme reference. The newly saved colours would only // leaves _active pointing at the PRE-edit Theme reference. The
// surface on the next RefreshActiveIfStale tick (1Hz-throttled, // newly saved colours would only surface on the next
// up to ~1s lag). Force-pull the freshly-cached Theme directly so // RefreshActiveIfStale tick (1Hz-throttled, up to ~1s lag).
// the post-Save UI sees the edit in the next frame. // Force-pull the freshly-cached Theme directly so the post-Save
// UI sees the edit in the next frame.
if (string.Equals(_active.Slug, targetSlug, StringComparison.OrdinalIgnoreCase)) if (string.Equals(_active.Slug, targetSlug, StringComparison.OrdinalIgnoreCase))
{ {
var reloaded = LoadCustomBySlug(targetSlug, out _); var reloaded = LoadCustomBySlug(targetSlug, out _);
@@ -408,15 +430,22 @@ public sealed class ThemeRegistry
} }
// Switch() falls back to DefaultSlug when neither built-in nor custom // Switch() falls back to DefaultSlug when neither built-in nor custom
// matches (ThemeRegistry.cs:128-132). Verify we actually landed on the // matches (see `Switch` default-slug fallback at the end of the
// intended theme before reporting success — a silent fallback to the // method). Verify we actually landed on the intended theme before
// default would otherwise mask a save that did persist the file but // reporting success — a silent fallback to the default would
// failed to become active (e.g. cache race on slow disks). // otherwise mask a save that did persist the file but failed to
// become active (e.g. cache race on slow disks).
if (!string.Equals(_active.Slug, targetSlug, StringComparison.OrdinalIgnoreCase)) if (!string.Equals(_active.Slug, targetSlug, StringComparison.OrdinalIgnoreCase))
{ {
// Log filename-only (not the full path) here — the path includes
// the user's home directory which counts as PII. Forensics-critical
// log calls above (path-escape detection) keep the full paths
// because diagnosing the escape needs the resolved target. Memory
// anchor: feedback_hellion_chat_changelog (v1.8.0 PII re-audit
// roadmap).
_logger?.LogWarning( _logger?.LogWarning(
"SaveEditingBuffer persisted {Path} but Switch landed on {Active} instead of {Target}", "SaveEditingBuffer persisted {File} but Switch landed on {Active} instead of {Target}",
targetPath, Path.GetFileName(targetPath),
_active.Slug, _active.Slug,
targetSlug targetSlug
); );
@@ -427,12 +456,20 @@ public sealed class ThemeRegistry
} }
catch (IOException ex) catch (IOException ex)
{ {
_logger?.LogWarning(ex, "I/O error saving editing buffer to {Path}", targetPath); _logger?.LogWarning(
ex,
"I/O error saving editing buffer to {File}",
Path.GetFileName(targetPath)
);
return false; return false;
} }
catch (UnauthorizedAccessException ex) catch (UnauthorizedAccessException ex)
{ {
_logger?.LogWarning(ex, "Access denied saving editing buffer to {Path}", targetPath); _logger?.LogWarning(
ex,
"Access denied saving editing buffer to {File}",
Path.GetFileName(targetPath)
);
return false; return false;
} }
catch (JsonException ex) catch (JsonException ex)
@@ -444,8 +481,8 @@ public sealed class ThemeRegistry
// — verify before saving and add the import if it's not yet present. // — verify before saving and add the import if it's not yet present.
_logger?.LogWarning( _logger?.LogWarning(
ex, ex,
"JSON serialisation failed for editing buffer at {Path}", "JSON serialisation failed for editing buffer at {File}",
targetPath Path.GetFileName(targetPath)
); );
return false; return false;
} }