From 79a3a7854f45b5e8863d7b712ec94e6a8804d0f8 Mon Sep 17 00:00:00 2001 From: Richard Date: Mon, 28 Sep 2026 10:45:46 +0200 Subject: [PATCH] Verlorene Updates erkennen statt warten: optimistische Pruefung beim Ersetzen, Konflikte im Audit und im Lagebild (#720) FileLedger merkt je Agent die gelesene Fassung einer Datei in gemeinsamen Bereichen. write, copy und Downloads (FTP, RocketChat) ersetzen eine Datei nur, wenn sie seit dem Lesen unveraendert ist; eine fremde Datei nicht ungelesen. append bleibt ungeprueft, macht aeltere Lesestaende aber ungueltig. Abgelehnte Schreibvorgaenge: eigener Audit-Status Conflict, Tabelle FileConflicts, Hotspots (>= 3 in 7 Tagen) in der Teamuebersicht. Co-Authored-By: Claude Opus 5.5 --- src/ClawdDotNet.App/AppHost.cs | 3 + src/ClawdDotNet.App/Services/TeamOverview.cs | 34 ++- src/ClawdDotNet.Core/Audit/AuditModels.cs | 9 +- src/ClawdDotNet.Core/Engine/AgentEngine.cs | 8 +- src/ClawdDotNet.Core/Files/FileLedger.cs | 232 ++++++++++++++++++ src/ClawdDotNet.Core/Storage/SqliteStorage.cs | 14 ++ .../Tools/AgentToolContext.cs | 6 + src/ClawdDotNet.Core/Tools/ToolResult.cs | 5 + src/ClawdDotNet.Manager.Web/wwwroot/app.css | 26 ++ .../wwwroot/i18n/de.json | 6 +- .../wwwroot/i18n/en.json | 6 +- .../wwwroot/views/team.js | 29 ++- src/ClawdDotNet.Tools.FTP/FtpTool.cs | 10 + src/ClawdDotNet.Tools.FileRW/FileRWTool.cs | 60 ++++- .../RocketChatTool.cs | 10 + src/ClawdDotNet.Web/Contracts/Dtos.cs | 6 +- src/ClawdDotNet.Web/Contracts/Mapping.cs | 4 +- .../Services/TeamOverviewTests.cs | 24 ++ .../Files/FileLedgerTests.cs | 188 ++++++++++++++ .../Infrastructure/FakeTool.cs | 8 + .../FileRW/LostUpdateTests.cs | 141 +++++++++++ 21 files changed, 818 insertions(+), 11 deletions(-) create mode 100644 src/ClawdDotNet.Core/Files/FileLedger.cs create mode 100644 tests/ClawdDotNet.Core.Tests/Files/FileLedgerTests.cs create mode 100644 tests/ClawdDotNet.Tools.Tests/FileRW/LostUpdateTests.cs diff --git a/src/ClawdDotNet.App/AppHost.cs b/src/ClawdDotNet.App/AppHost.cs index 3320402..af0d59c 100644 --- a/src/ClawdDotNet.App/AppHost.cs +++ b/src/ClawdDotNet.App/AppHost.cs @@ -598,6 +598,9 @@ public sealed class AppHost : IAsyncDisposable foreach (var user in tools.GetAll().OfType()) user.Sandbox = sandbox; + // #720: which version of a shared file each agent read — a write cannot silently lose another. + engine.Files = new Core.Files.FileLedger(storage); + // #712: Foreign runtimes. The application itself is the launcher inside the sandbox // (network through the proxy, secrets from a file) and the MCP bridge back to us. var mcpGrants = new Core.Mcp.McpGrants(storage); diff --git a/src/ClawdDotNet.App/Services/TeamOverview.cs b/src/ClawdDotNet.App/Services/TeamOverview.cs index 44d9531..288635e 100644 --- a/src/ClawdDotNet.App/Services/TeamOverview.cs +++ b/src/ClawdDotNet.App/Services/TeamOverview.cs @@ -67,7 +67,8 @@ public sealed record TeamSnapshot( IReadOnlyList Members, IReadOnlyList RecentLinks, DateTime AtUtc, - IReadOnlyList? Units = null); + IReadOnlyList? Units = null, + IReadOnlyList? ConflictHotspots = null); /// /// #711: The team overview — who is working, who reached whom, where something is stuck. @@ -105,6 +106,12 @@ public sealed class TeamOverview( /// Without history, a run counts as stuck after this long. public static readonly TimeSpan StuckWithoutHistory = TimeSpan.FromMinutes(15); + /// #720: How far back conflicts count for a hotspot… + public static readonly TimeSpan HotspotWindow = TimeSpan.FromDays(7); + + /// …and how many make one. One conflict is an accident; three in a week are a pattern. + public const int HotspotMinimum = 3; + private readonly TimeProvider _clock = clock ?? TimeProvider.System; public async Task SnapshotAsync(CancellationToken ct) @@ -151,7 +158,30 @@ public sealed class TeamOverview( org.UnitOf(agent.AgentId)?.Id, org.LedBy(agent.AgentId) is not null)); } - return new TeamSnapshot(members, links, now, RollUp(org, members)); + // #720: files where agents keep getting in each other's way — for a human to decide + // whether they should be split, appended to, or become a table. + var hotspots = engine?.Files is { } files + ? (await files.HotspotsAsync(now - HotspotWindow, HotspotMinimum, 10, ct)) + .Select(h => h with { Path = Readable(h.Path, configured) }).ToList() + : []; + + return new TeamSnapshot(members, links, now, RollUp(org, members), hotspots); + } + + /// + /// #720: A file path as a human finds it — relative to the instance's Agents folder, + /// where the shared workspaces live. A path elsewhere stays as it is. + /// + public static string Readable(string path, IReadOnlyList agents) + { + var shared = agents.Select(a => a.SharedWorkspacePath).FirstOrDefault(p => !string.IsNullOrWhiteSpace(p)); + if (shared is null + || Path.GetDirectoryName(Path.TrimEndingDirectorySeparator(Path.GetFullPath(shared))) is not { } agentsDir) + return path; + var relative = Path.GetRelativePath(agentsDir, path); + return relative.StartsWith("..", StringComparison.Ordinal) || Path.IsPathRooted(relative) + ? path + : relative.Replace(Path.DirectorySeparatorChar, '/'); } /// diff --git a/src/ClawdDotNet.Core/Audit/AuditModels.cs b/src/ClawdDotNet.Core/Audit/AuditModels.cs index 893bba2..7495bec 100644 --- a/src/ClawdDotNet.Core/Audit/AuditModels.cs +++ b/src/ClawdDotNet.Core/Audit/AuditModels.cs @@ -37,7 +37,14 @@ public enum AuditStatus /// -Eintrag offen, und jeder gewöhnliche Abbruch sähe beim /// nächsten Start aus wie ein Absturz — bis niemand mehr hinsieht. /// - Cancelled + Cancelled, + + /// + /// #720: The write was refused because the file changed since the agent read it, or + /// another agent wrote it and this one never read it. Its own status, so the conflicts + /// can be counted — frequent ones say the file should be stored differently. + /// + Conflict } /// diff --git a/src/ClawdDotNet.Core/Engine/AgentEngine.cs b/src/ClawdDotNet.Core/Engine/AgentEngine.cs index e73a97d..aca11d7 100644 --- a/src/ClawdDotNet.Core/Engine/AgentEngine.cs +++ b/src/ClawdDotNet.Core/Engine/AgentEngine.cs @@ -60,6 +60,9 @@ public sealed class AgentEngine : IAgentMessageRouter, Staging.IFrozenCallExecut /// public Budget.ISpecialBudgetRepository? SpecialBudgets { get; set; } + /// #720: Which version of a shared file each agent read — so a write cannot silently lose another. + public Files.FileLedger? Files { get; set; } + /// #654: The sandbox tools run code in. public Sandbox.ISandboxRunner? Sandbox { get; set; } @@ -1665,7 +1668,7 @@ public sealed class AgentEngine : IAgentMessageRouter, Staging.IFrozenCallExecut if (!result.Success) { - await Audit(AuditStatus.Error, result.ErrorMessage ?? ""); + await Audit(result.IsConflict ? AuditStatus.Conflict : AuditStatus.Error, result.ErrorMessage ?? ""); return JsonSerializer.Serialize(new { error = result.ErrorMessage }); } @@ -1856,6 +1859,7 @@ public sealed class AgentEngine : IAgentMessageRouter, Staging.IFrozenCallExecut Cases = CaseDesk, SpecialBudgets = SpecialBudgets, Sandbox = Sandbox, + Files = Files, // #697: Where something an agent hands on came from — known to the engine, not claimed by the agent. RunId = runId, RunTaskId = runId is null ? null : Activity.Find(runId)?.TaskId @@ -1890,7 +1894,7 @@ public sealed class AgentEngine : IAgentMessageRouter, Staging.IFrozenCallExecut var context = BuildToolContext(config, _instanceId, tool, ct, runId: runId) with { FreigabeGeprueft = true }; var result = await agentTool.ExecuteAsync(input, context, ct); - var status = result.Success ? AuditStatus.Ok : AuditStatus.Error; + var status = result.Success ? AuditStatus.Ok : result.IsConflict ? AuditStatus.Conflict : AuditStatus.Error; await RecordAuditAsync(runId, config, AuditSource.Approval, tool, argumentsJson, status, result.Success ? "Freigegeben ausgeführt" : (result.ErrorMessage ?? ""), sw.ElapsedMilliseconds); diff --git a/src/ClawdDotNet.Core/Files/FileLedger.cs b/src/ClawdDotNet.Core/Files/FileLedger.cs new file mode 100644 index 0000000..54cfd40 --- /dev/null +++ b/src/ClawdDotNet.Core/Files/FileLedger.cs @@ -0,0 +1,232 @@ +using System.Collections.Concurrent; +using System.Globalization; +using System.Security.Cryptography; +using ClawdDotNet.Core.Storage; + +namespace ClawdDotNet.Core.Files; + +/// #720: A write that was refused because it would have lost someone's change. +public sealed record FileConflict( + string Path, + string Workspace, + string AgentId, + string? RunId, + string? OtherAgentId, + string? OtherRunId, + string Kind, + DateTime OccurredAtUtc) +{ + /// The file changed after this agent read it. + public const string ChangedSinceRead = "changed-since-read"; + + /// Another agent wrote the file, and this one never read it. + public const string NotRead = "not-read"; + + /// What the agent is told — what happened, and what to do instead. + public string Message(string displayPath) + { + var who = OtherAgentId is null ? "someone else" : $"agent '{OtherAgentId}'"; + return Kind == NotRead + ? $"Conflict: {displayPath} was last written by {who}, and you have not read it. " + + "Read it first and make your change on its current content — or use 'append' if you only add to it." + : $"Conflict: {displayPath} was changed by {who} since you read it. Your write would overwrite that change. " + + "Read it again and make your change on the current content — or use 'append' if you only add to it."; + } +} + +/// #720: A file that keeps running into conflicts — the candidate for a different kind of storage. +public sealed record FileConflictHotspot(string Path, string Workspace, int Conflicts, DateTime LastAtUtc, IReadOnlyList Agents); + +/// +/// #720: Lost updates in shared workspaces, found instead of waited for. +/// +/// Optimistic, not locked. A lock held across model steps lasts minutes, is left +/// behind when a run breaks off, and makes the waiting agents burn steps. Instead the +/// instance remembers which version of a file each agent read. A write that replaces a file +/// compares the file as it is now with that version; if someone changed it in between, the +/// write is refused and the agent reads again. The agent needs no parameter for this, so it +/// holds for a cheap model that would forget one. +/// +/// What is remembered where. Reads and last writers live in memory: after a +/// restart there is no run that could build on an earlier read, and a file whose writer is +/// unknown may be overwritten as before. Conflicts are kept in the database — they are what +/// shows which files should become something else (split per agent, append-only, a table). +/// +/// Appending is not checked: it loses nothing. It is noted, though — whoever read the +/// file before the append does not know the appended part, and a later replace by them is a +/// conflict. That is the point. +/// +public sealed class FileLedger +{ + private sealed record Mark(string Hash, string? RunId, DateTime AtUtc, string AgentId); + + private readonly ConcurrentDictionary<(string Path, string Agent), Mark> _reads = new(); + private readonly ConcurrentDictionary _writers = new(); + private readonly SqliteStorage? _storage; + private readonly TimeProvider _clock; + + public FileLedger(SqliteStorage? storage = null, TimeProvider? clock = null) + { + _storage = storage; + _clock = clock ?? TimeProvider.System; + } + + /// The SHA-256 of a file's bytes; null if it does not exist. + public static string? HashOf(string path) + { + if (!File.Exists(path)) + return null; + using var stream = new FileStream(path, FileMode.Open, FileAccess.Read, FileShare.ReadWrite | FileShare.Delete); + return Convert.ToHexStringLower(SHA256.HashData(stream)); + } + + /// An agent read the file in this version. + public void NoteRead(string agentId, string path, string? hash) + { + if (hash is null) + return; + _reads[(Key(path), Agent(agentId))] = new Mark(hash, null, Now, agentId); + } + + /// + /// Whether an agent may replace the file, given what it is now. null: yes. Pure + /// apart from the lookups — the caller holds the file's lock while it checks and writes. + /// + public FileConflict? CheckReplace(string agentId, string? runId, string path, string workspace, string? currentHash) + { + if (currentHash is null) + return null; // a new file replaces nothing + + var key = Key(path); + _writers.TryGetValue(key, out var writer); + // The writer we know of is the one who made the file as it is now — otherwise + // someone we do not know of (a human, a tool without the ledger) changed it since. + var knownWriter = writer is not null && writer.Hash == currentHash ? writer : null; + + if (_reads.TryGetValue((key, Agent(agentId)), out var read)) + { + return read.Hash == currentHash + ? null + : Conflict(FileConflict.ChangedSinceRead); + } + + return knownWriter is not null && !SameAgent(knownWriter.AgentId, agentId) + ? Conflict(FileConflict.NotRead) + : null; + + FileConflict Conflict(string kind) => new( + path, workspace, agentId, runId, + knownWriter is not null && !SameAgent(knownWriter.AgentId, agentId) ? knownWriter.AgentId : null, + knownWriter is not null && !SameAgent(knownWriter.AgentId, agentId) ? knownWriter.RunId : null, + kind, Now); + } + + /// The agent wrote the whole file: it is the last writer and knows what it wrote. + public void NoteReplace(string agentId, string? runId, string path, string? newHash) + { + if (newHash is null) + return; + var key = Key(path); + var mark = new Mark(newHash, runId, Now, agentId); + _writers[key] = mark; + _reads[(key, Agent(agentId))] = mark; + } + + /// + /// The agent appended. It is the last writer; its own read stays valid only if it knew + /// the file as it was before — everyone else's read is now of an older version. + /// + public void NoteAppend(string agentId, string? runId, string path, string? hashBefore, string? hashAfter) + { + if (hashAfter is null) + return; + var key = Key(path); + var mark = new Mark(hashAfter, runId, Now, agentId); + _writers[key] = mark; + var own = (key, Agent(agentId)); + if (hashBefore is null || (_reads.TryGetValue(own, out var read) && read.Hash == hashBefore)) + _reads[own] = mark; + } + + /// Keeps the conflict for the analysis. Best effort — the refusal itself does not depend on it. + public async Task RecordAsync(FileConflict conflict, CancellationToken ct) + { + if (_storage is null) + return; + try + { + await _storage.WriteAsync(async conn => + { + using var cmd = conn.CreateCommand(); + cmd.CommandText = """ + INSERT INTO FileConflicts (Path, Workspace, AgentId, RunId, OtherAgentId, OtherRunId, Kind, OccurredAt) + VALUES (@path, @workspace, @agent, @run, @other, @otherRun, @kind, @at) + """; + cmd.Parameters.AddWithValue("@path", conflict.Path); + cmd.Parameters.AddWithValue("@workspace", conflict.Workspace); + cmd.Parameters.AddWithValue("@agent", conflict.AgentId); + cmd.Parameters.AddWithValue("@run", (object?)conflict.RunId ?? DBNull.Value); + cmd.Parameters.AddWithValue("@other", (object?)conflict.OtherAgentId ?? DBNull.Value); + cmd.Parameters.AddWithValue("@otherRun", (object?)conflict.OtherRunId ?? DBNull.Value); + cmd.Parameters.AddWithValue("@kind", conflict.Kind); + cmd.Parameters.AddWithValue("@at", conflict.OccurredAtUtc.ToString("O", CultureInfo.InvariantCulture)); + await cmd.ExecuteNonQueryAsync(ct); + }, ct); + } + catch (Exception ex) when (ex is not OperationCanceledException) + { + // The conflict was refused all the same; only the count is missing one. + } + } + + /// The files with at least conflicts since , most first. + public async Task> HotspotsAsync(DateTime sinceUtc, int minimum, int limit, CancellationToken ct) + { + if (_storage is null) + return []; + + await using var conn = await _storage.OpenConnectionAsync(ct); + using var cmd = conn.CreateCommand(); + cmd.CommandText = """ + SELECT Path, Workspace, AgentId, OtherAgentId, OccurredAt + FROM FileConflicts + WHERE OccurredAt >= @since + """; + cmd.Parameters.AddWithValue("@since", sinceUtc.ToString("O", CultureInfo.InvariantCulture)); + + var rows = new List<(string Path, string Workspace, string Agent, string? Other, DateTime At)>(); + await using (var reader = await cmd.ExecuteReaderAsync(ct)) + { + while (await reader.ReadAsync(ct)) + { + rows.Add((reader.GetString(0), reader.GetString(1), reader.GetString(2), + reader.IsDBNull(3) ? null : reader.GetString(3), + DateTime.Parse(reader.GetString(4), CultureInfo.InvariantCulture, DateTimeStyles.RoundtripKind))); + } + } + + return rows + .GroupBy(r => (r.Path, r.Workspace)) + .Select(g => new FileConflictHotspot( + g.Key.Path, g.Key.Workspace, g.Count(), g.Max(r => r.At), + g.SelectMany(r => r.Other is null ? new[] { r.Agent } : new[] { r.Agent, r.Other }) + .Distinct(StringComparer.OrdinalIgnoreCase).Order(StringComparer.OrdinalIgnoreCase).ToList())) + .Where(h => h.Conflicts >= minimum) + .OrderByDescending(h => h.Conflicts).ThenByDescending(h => h.LastAtUtc) + .Take(limit) + .ToList(); + } + + private DateTime Now => _clock.GetUtcNow().UtcDateTime; + + private static bool SameAgent(string a, string b) => string.Equals(a, b, StringComparison.OrdinalIgnoreCase); + + private static string Agent(string agentId) => agentId.ToLowerInvariant(); + + /// One key per file, however the path was written; case-insensitive where the file system is. + private static string Key(string path) + { + var full = Path.GetFullPath(path); + return OperatingSystem.IsWindows() || OperatingSystem.IsMacOS() ? full.ToLowerInvariant() : full; + } +} diff --git a/src/ClawdDotNet.Core/Storage/SqliteStorage.cs b/src/ClawdDotNet.Core/Storage/SqliteStorage.cs index 9744b97..eaab74d 100644 --- a/src/ClawdDotNet.Core/Storage/SqliteStorage.cs +++ b/src/ClawdDotNet.Core/Storage/SqliteStorage.cs @@ -374,6 +374,20 @@ public sealed class SqliteStorage Json TEXT NOT NULL ); + -- #720: Writes refused because they would have lost someone's change. + CREATE TABLE IF NOT EXISTS FileConflicts ( + Id INTEGER PRIMARY KEY AUTOINCREMENT, + Path TEXT NOT NULL, + Workspace TEXT NOT NULL, + AgentId TEXT NOT NULL, + RunId TEXT NULL, + OtherAgentId TEXT NULL, + OtherRunId TEXT NULL, + Kind TEXT NOT NULL, + OccurredAt TEXT NOT NULL + ); + CREATE INDEX IF NOT EXISTS IX_FileConflicts_OccurredAt ON FileConflicts (OccurredAt); + -- #713: Personal tokens for the MCP server — only the hash is kept. CREATE TABLE IF NOT EXISTS McpTokens ( Id TEXT PRIMARY KEY, diff --git a/src/ClawdDotNet.Core/Tools/AgentToolContext.cs b/src/ClawdDotNet.Core/Tools/AgentToolContext.cs index 74933fd..0a3617b 100644 --- a/src/ClawdDotNet.Core/Tools/AgentToolContext.cs +++ b/src/ClawdDotNet.Core/Tools/AgentToolContext.cs @@ -55,6 +55,12 @@ public sealed record AgentToolContext( /// #717: Special budgets — set up by an approved request_budget. public Budget.ISpecialBudgetRepository? SpecialBudgets { get; init; } + /// + /// #720: Which version of a shared file each agent read. Tools that replace files in a + /// shared workspace check it first. null in tests and outside the engine: no check. + /// + public Files.FileLedger? Files { get; init; } + /// #697: The run this call belongs to — for stamping where something came from. public string? RunId { get; init; } diff --git a/src/ClawdDotNet.Core/Tools/ToolResult.cs b/src/ClawdDotNet.Core/Tools/ToolResult.cs index a7ae08a..7fbb8dc 100644 --- a/src/ClawdDotNet.Core/Tools/ToolResult.cs +++ b/src/ClawdDotNet.Core/Tools/ToolResult.cs @@ -9,4 +9,9 @@ public sealed record ToolResult( public static ToolResult Ok(string content) => new(true, content); public static ToolResult Fail(string error) => new(false, "", error); + + /// #720: Refused because it would have lost another write — audited as a conflict, not as an error. + public bool IsConflict { get; init; } + + public static ToolResult Conflict(string error) => new(false, "", error) { IsConflict = true }; } diff --git a/src/ClawdDotNet.Manager.Web/wwwroot/app.css b/src/ClawdDotNet.Manager.Web/wwwroot/app.css index a8db5da..1169094 100644 --- a/src/ClawdDotNet.Manager.Web/wwwroot/app.css +++ b/src/ClawdDotNet.Manager.Web/wwwroot/app.css @@ -869,6 +869,32 @@ button.klein { min-width: 0; } +/* #720: files with repeated write conflicts, above the team canvas */ +.team-konflikte { + border: 1px solid var(--divider); + border-left: 4px solid var(--gefahr, #c0392b); + background: var(--surface); + padding: 10px 14px; + margin-bottom: 12px; +} + +.team-konflikte[hidden] { + display: none; +} + +.team-konflikte p { + margin: 4px 0 6px; +} + +.team-konflikte ul { + margin: 0; + padding-left: 18px; +} + +.team-konflikte code { + word-break: break-all; +} + /* #712: the runtime card */ .laufzeit-feld { display: grid; diff --git a/src/ClawdDotNet.Manager.Web/wwwroot/i18n/de.json b/src/ClawdDotNet.Manager.Web/wwwroot/i18n/de.json index 74715d7..f8a4ee8 100644 --- a/src/ClawdDotNet.Manager.Web/wwwroot/i18n/de.json +++ b/src/ClawdDotNet.Manager.Web/wwwroot/i18n/de.json @@ -982,5 +982,9 @@ "runtime.oauthToken": "Abo-Token", "runtime.oauthTokenHint": "Erzeugt mit 'claude setup-token' auf einem Rechner, an dem du angemeldet bist.", "runtime.keepStored": "hinterlegt – leer lassen zum Behalten", - "runtime.accountSaved": "Claude-Code-Konto gespeichert." + "runtime.accountSaved": "Claude-Code-Konto gespeichert.", + + "team.conflicts.title": "Dateien, in denen sich Agenten gegenseitig überschreiben", + "team.conflicts.hint": "Hier wurden Schreibvorgänge abgelehnt, weil sie die Änderung eines anderen Agenten verloren hätten. Passiert das oft, sollte die Ablage anders werden: eine Datei je Agent oder Tag, Anhängen statt Ersetzen oder eine Tabelle.", + "team.conflicts.count": "{n} Konflikte in 7 Tagen, zwischen {agents}; zuletzt {time}" } diff --git a/src/ClawdDotNet.Manager.Web/wwwroot/i18n/en.json b/src/ClawdDotNet.Manager.Web/wwwroot/i18n/en.json index 89a1811..7602425 100644 --- a/src/ClawdDotNet.Manager.Web/wwwroot/i18n/en.json +++ b/src/ClawdDotNet.Manager.Web/wwwroot/i18n/en.json @@ -982,5 +982,9 @@ "runtime.oauthToken": "Subscription token", "runtime.oauthTokenHint": "Created with 'claude setup-token' on a machine where you are signed in.", "runtime.keepStored": "stored — leave empty to keep", - "runtime.accountSaved": "Claude Code account saved." + "runtime.accountSaved": "Claude Code account saved.", + + "team.conflicts.title": "Files agents keep overwriting each other in", + "team.conflicts.hint": "Writes were refused here because they would have lost another agent's change. If it keeps happening, change how the file is kept: one file per agent or day, appending instead of replacing, or a table.", + "team.conflicts.count": "{n} conflicts in 7 days, between {agents}; last {time}" } diff --git a/src/ClawdDotNet.Manager.Web/wwwroot/views/team.js b/src/ClawdDotNet.Manager.Web/wwwroot/views/team.js index c0ba05b..452c814 100644 --- a/src/ClawdDotNet.Manager.Web/wwwroot/views/team.js +++ b/src/ClawdDotNet.Manager.Web/wwwroot/views/team.js @@ -106,6 +106,7 @@ let matchInfo = null; let zoomValue = null; let panel = null; let summary = null; +let hotspots = null; let unsubscribe = null; let refreshTimer = null; @@ -151,8 +152,12 @@ export async function renderTeam() { panel = erstelleElement("aside", { klasse: "team-seitenleiste" }); panel.hidden = true; + // #720: files agents keep overwriting each other in — hidden while there are none. + hotspots = erstelleElement("div", { klasse: "team-konflikte", role: "status" }); + hotspots.hidden = true; + const area = erstelleElement("div", { klasse: "team-arbeitsflaeche" }, buildTree(), canvas, panel); - container.replaceChildren(erstelleElement("div", { klasse: "team-ansicht" }, head, area, buildLegend())); + container.replaceChildren(erstelleElement("div", { klasse: "team-ansicht" }, head, hotspots, area, buildLegend())); if (!holeAktiveInstanz()) { canvas.replaceChildren(erstelleElement("div", { klasse: "hinweis-zentriert", text: t("team.noInstance") })); @@ -1068,6 +1073,28 @@ function renderSummary() { summary.replaceChildren( ...(parts.length ? parts : [erstelleElement("span", { klasse: "gedaempft", text: t("team.empty") })]) ); + renderHotspots(); +} + +/** + * #720: Files with repeated write conflicts in the last week. Detecting is code; what to do + * about it — split the file, append instead, make it a table — is a human's call. + */ +function renderHotspots() { + if (!hotspots || !snapshot) return; + const list = snapshot.conflictHotspots ?? []; + hotspots.hidden = list.length === 0; + if (list.length === 0) { + hotspots.replaceChildren(); + return; + } + hotspots.replaceChildren( + erstelleElement("strong", { text: `⚠ ${t("team.conflicts.title")}` }), + erstelleElement("p", { klasse: "gedaempft text-klein", text: t("team.conflicts.hint") }), + erstelleElement("ul", {}, + ...list.map((h) => erstelleElement("li", {}, + erstelleElement("code", { text: h.path }), + ` — ${t("team.conflicts.count", { n: h.conflicts, agents: h.agents.join(", "), time: formatiereDatumZeit(h.lastAtUtc) })}`)))); } /** The card tones by the names the badges use. */ diff --git a/src/ClawdDotNet.Tools.FTP/FtpTool.cs b/src/ClawdDotNet.Tools.FTP/FtpTool.cs index f87e0e0..ecd3774 100644 --- a/src/ClawdDotNet.Tools.FTP/FtpTool.cs +++ b/src/ClawdDotNet.Tools.FTP/FtpTool.cs @@ -317,10 +317,20 @@ public sealed class FTPTool : IAgentTool { var (localFull, wsLabel) = ResolveLocalPath(localRel, context, write: true); + // #720: a download over a shared file replaces it — not over a change this agent has not seen. + var guarded = wsLabel != WorkspaceLayout.Personal && context.Files is not null; + if (guarded && context.Files!.CheckReplace(context.AgentId, context.RunId, localFull, wsLabel, Core.Files.FileLedger.HashOf(localFull)) is { } conflict) + { + await context.Files.RecordAsync(conflict, ct); + return ToolResult.Conflict(conflict.Message(localRel)); + } + var localDir = Path.GetDirectoryName(localFull); if (localDir != null && !Directory.Exists(localDir)) Directory.CreateDirectory(localDir); var status = await client.DownloadFile(localFull, remote, FtpLocalExists.Overwrite, FtpVerify.None, null, ct); + if (guarded && status == FtpStatus.Success) + context.Files!.NoteReplace(context.AgentId, context.RunId, localFull, Core.Files.FileLedger.HashOf(localFull)); return status == FtpStatus.Success ? ToolResult.Ok($"[{wsLabel} Workspace] File successfully downloaded: {remote} → {localRel}") : ToolResult.Fail($"Download failed: {status}"); diff --git a/src/ClawdDotNet.Tools.FileRW/FileRWTool.cs b/src/ClawdDotNet.Tools.FileRW/FileRWTool.cs index 2c4803f..8aeb45e 100644 --- a/src/ClawdDotNet.Tools.FileRW/FileRWTool.cs +++ b/src/ClawdDotNet.Tools.FileRW/FileRWTool.cs @@ -1,3 +1,4 @@ +using ClawdDotNet.Core.Files; using ClawdDotNet.Core.Config; using ClawdDotNet.Core.Organisation; using System.Collections.Concurrent; @@ -310,6 +311,41 @@ public sealed class FileRWTool : IAgentTool, IToolStateProbe || relative.StartsWith(TasksFolder + "/", StringComparison.OrdinalIgnoreCase); } + // ─── Lost updates (#720) ─── + + /// Only workspaces more than one agent can write to — never the personal one. + private static bool Guarded(Area area, AgentToolContext context) => !area.IsPersonal && context.Files is not null; + + private static void NoteRead(string path, Area area, AgentToolContext context) + { + if (Guarded(area, context)) + context.Files!.NoteRead(context.AgentId, path, FileLedger.HashOf(path)); + } + + private static void NoteReplace(string path, Area area, AgentToolContext context) + { + if (Guarded(area, context)) + context.Files!.NoteReplace(context.AgentId, context.RunId, path, FileLedger.HashOf(path)); + } + + /// + /// The refusal when replacing the file would lose a change this agent has not seen — + /// kept for the analysis, audited as a conflict. null: go ahead. Called under the + /// file's lock, so nothing can change between the check and the write. + /// + private static async Task RefuseLostUpdateAsync( + string path, string displayPath, Area area, AgentToolContext context, CancellationToken ct) + { + if (!Guarded(area, context)) + return null; + if (context.Files!.CheckReplace(context.AgentId, context.RunId, path, area.Name, FileLedger.HashOf(path)) is not { } conflict) + return null; + + context.Logger.LogInformation("Write refused, lost update: {Path} by {Agent} ({Kind})", displayPath, context.AgentId, conflict.Kind); + await context.Files.RecordAsync(conflict, ct); + return ToolResult.Conflict(conflict.Message(displayPath)); + } + private static List NormaliseProtectedPaths(IEnumerable paths) => paths .Select(p => p.Replace('\\', '/').Trim().Trim('/')) @@ -504,6 +540,7 @@ public sealed class FileRWTool : IAgentTool, IToolStateProbe try { var content = await File.ReadAllTextAsync(path, ct); + NoteRead(path, area, context); var relativePath = Path.GetRelativePath(rootPath, path); return ToolResult.Ok($"[{area.Name}:/{relativePath}]\n{content}"); } @@ -561,7 +598,11 @@ public sealed class FileRWTool : IAgentTool, IToolStateProbe var sperre = GetFileLock(pfad); await sperre.WaitAsync(ct); string inhalt; - try { inhalt = await File.ReadAllTextAsync(pfad, ct); } + try + { + inhalt = await File.ReadAllTextAsync(pfad, ct); + NoteRead(pfad, area, context); + } finally { sperre.Release(); } text.Append($"\n--- {area.Name}:/{relativ} ---\n").Append(inhalt.TrimEnd()).Append('\n'); @@ -665,6 +706,10 @@ public sealed class FileRWTool : IAgentTool, IToolStateProbe $"Tip: Use 'stock_add' for structured data points."); } + // #720: not over a change this agent has not seen. + if (await RefuseLostUpdateAsync(path, $"{area.Name}:/{Path.GetRelativePath(rootPath, path).Replace('\\', '/')}", area, context, ct) is { } conflict) + return conflict; + var dir = Path.GetDirectoryName(path); if (dir != null && !Directory.Exists(dir)) Directory.CreateDirectory(dir); @@ -674,6 +719,7 @@ public sealed class FileRWTool : IAgentTool, IToolStateProbe // schützte also ihre eigenen Konfigurationsdateien, die Arbeitsergebnisse der // Agenten aber nicht. await AtomicFile.WriteAllTextAsync(path, content, new UTF8Encoding(false), ct); + NoteReplace(path, area, context); return ToolResult.Ok($"[{area.Name} Workspace] File successfully written: {area.Name}:/{Path.GetRelativePath(rootPath, path)}"); } finally @@ -695,10 +741,17 @@ public sealed class FileRWTool : IAgentTool, IToolStateProbe // die ganze Datei neu geschrieben wird — bei einer großen Protokolldatei wäre // das der falsche Handel. Hier bleibt ein Riss möglich; genau dafür gibt es // die Zustandsprüfung in #555. + // #720: appending loses nothing and is not checked — but noted, so whoever read + // the file before does not replace it without the appended part. + var before = Guarded(area, context) ? FileLedger.HashOf(path) : null; + // #575: Neu entstehende Dateien gehoeren nur dem Besitzer (0600). await using (var stream = ClawdDotNet.Core.Storage.OwnerOnlyFile.OpenAppend(path)) await using (var writer = new StreamWriter(stream, new UTF8Encoding(false))) await writer.WriteAsync(content.AsMemory(), ct); + + if (Guarded(area, context)) + context.Files!.NoteAppend(context.AgentId, context.RunId, path, before, FileLedger.HashOf(path)); return ToolResult.Ok($"[{area.Name} Workspace] Content successfully appended to: {area.Name}:/{Path.GetRelativePath(rootPath, path)}"); } finally @@ -909,10 +962,15 @@ public sealed class FileRWTool : IAgentTool, IToolStateProbe await second.WaitAsync(ct); try { + // #720: copying over a file replaces it — the same check as a write. + if (await RefuseLostUpdateAsync(destPath, $"{destArea.Name}:/{Path.GetRelativePath(destRootPath, destPath).Replace('\\', '/')}", destArea, context, ct) is { } conflict) + return conflict; + var destDir = Path.GetDirectoryName(destPath); if (destDir != null && !Directory.Exists(destDir)) Directory.CreateDirectory(destDir); await Task.Run(() => File.Copy(sourcePath, destPath, overwrite: !IsPathProtected(destPath, destRootPath, destArea)), ct); + NoteReplace(destPath, destArea, context); var srcLabel = $"{sourceArea.Name}:/{Path.GetRelativePath(sourceRootPath, sourcePath)}"; var dstLabel = $"{destArea.Name}:/{Path.GetRelativePath(destRootPath, destPath)}"; diff --git a/src/ClawdDotNet.Tools.RocketChat/RocketChatTool.cs b/src/ClawdDotNet.Tools.RocketChat/RocketChatTool.cs index 4644f3d..0234037 100644 --- a/src/ClawdDotNet.Tools.RocketChat/RocketChatTool.cs +++ b/src/ClawdDotNet.Tools.RocketChat/RocketChatTool.cs @@ -255,9 +255,19 @@ public sealed class RocketChatTool : IAgentTool, IToolJobProvider if (file is null) return ToolResult.Fail(pathError!); + // #720: a download over a shared file replaces it — not over a change this agent has not seen. + var guarded = file.Label != WorkspaceLayout.Personal && context.Files is not null; + if (guarded && context.Files!.CheckReplace(context.AgentId, context.RunId, file.FullPath, file.Label, Core.Files.FileLedger.HashOf(file.FullPath)) is { } conflict) + { + await context.Files.RecordAsync(conflict, ct); + return ToolResult.Conflict(conflict.Message($"{file.Label}:{targetName}")); + } + Directory.CreateDirectory(Path.GetDirectoryName(file.FullPath)!); var bytes = await client.DownloadToFileAsync(fileId, file.FullPath, config.MaxDownloadBytes, ct); + if (guarded) + context.Files!.NoteReplace(context.AgentId, context.RunId, file.FullPath, Core.Files.FileLedger.HashOf(file.FullPath)); return ToolResult.Ok(JsonSerializer.Serialize(new { diff --git a/src/ClawdDotNet.Web/Contracts/Dtos.cs b/src/ClawdDotNet.Web/Contracts/Dtos.cs index d93003a..109095b 100644 --- a/src/ClawdDotNet.Web/Contracts/Dtos.cs +++ b/src/ClawdDotNet.Web/Contracts/Dtos.cs @@ -189,7 +189,11 @@ public sealed record TeamSnapshotDto( IReadOnlyList Members, IReadOnlyList Links, DateTime AtUtc, - IReadOnlyList Units); + IReadOnlyList Units, + IReadOnlyList ConflictHotspots); + +/// #720: A file agents keep overwriting each other in — shown with a warning. The path is relative to the instance's Agents folder. +public sealed record ConflictHotspotDto(string Workspace, string Path, int Conflicts, DateTime LastAtUtc, IReadOnlyList Agents); /// /// #711, stage 1: A unit with the states of everyone in it and below, counted by the diff --git a/src/ClawdDotNet.Web/Contracts/Mapping.cs b/src/ClawdDotNet.Web/Contracts/Mapping.cs index 2eb1da5..24a6ee5 100644 --- a/src/ClawdDotNet.Web/Contracts/Mapping.cs +++ b/src/ClawdDotNet.Web/Contracts/Mapping.cs @@ -101,7 +101,9 @@ internal static class Mapping (snapshot.Units ?? []).Select(u => new TeamUnitDto( u.Id, u.Name, u.ParentId, u.Path, u.LeaderAgentId, u.Goal, u.Agents, u.States.ToDictionary(s => s.Key.ToString(), s => s.Value), - u.PendingApprovals, u.Worst.ToString())).ToList()); + u.PendingApprovals, u.Worst.ToString())).ToList(), + (snapshot.ConflictHotspots ?? []).Select(h => new ConflictHotspotDto( + h.Workspace, h.Path, h.Conflicts, h.LastAtUtc, h.Agents)).ToList()); public static ActiveRunDto ToDto(this Core.Activity.ActiveRun run) => new(run.RunId, run.AgentId, run.Source, run.TaskId, run.CalledBy, run.StartedAtUtc); diff --git a/tests/ClawdDotNet.App.Tests/Services/TeamOverviewTests.cs b/tests/ClawdDotNet.App.Tests/Services/TeamOverviewTests.cs index 0378157..85b29f7 100644 --- a/tests/ClawdDotNet.App.Tests/Services/TeamOverviewTests.cs +++ b/tests/ClawdDotNet.App.Tests/Services/TeamOverviewTests.cs @@ -162,4 +162,28 @@ public sealed class TeamOverviewTests public void Without_units_there_is_nothing_to_roll_up() => TeamOverview.RollUp(new OrgChart([], [new AgentConfig { AgentId = "a" }]), [Member("a", null, TeamState.Problem)]) .ShouldBeEmpty(); + + // ─── #720: conflict hotspots ─── + + [Fact] + public void A_hotspot_is_shown_relative_to_the_agents_folder() + { + var agents = Path.Combine(Path.GetTempPath(), "inst", "Agents"); + var configured = new List { new() { AgentId = "a", SharedWorkspacePath = Path.Combine(agents, "SharedWorkspace") } }; + + TeamOverview.Readable(Path.Combine(agents, "SharedWorkspaces", "articles", "list.csv"), configured) + .ShouldBe("SharedWorkspaces/articles/list.csv"); + TeamOverview.Readable(Path.Combine(agents, "SharedWorkspace", "plan.md"), configured) + .ShouldBe("SharedWorkspace/plan.md"); + } + + [Fact] + public void A_path_elsewhere_stays_as_it_is() + { + var configured = new List { new() { AgentId = "a", SharedWorkspacePath = Path.Combine(Path.GetTempPath(), "inst", "Agents", "SharedWorkspace") } }; + var elsewhere = Path.Combine(Path.GetTempPath(), "other", "x.md"); + + TeamOverview.Readable(elsewhere, configured).ShouldBe(elsewhere); + TeamOverview.Readable(elsewhere, []).ShouldBe(elsewhere); + } } diff --git a/tests/ClawdDotNet.Core.Tests/Files/FileLedgerTests.cs b/tests/ClawdDotNet.Core.Tests/Files/FileLedgerTests.cs new file mode 100644 index 0000000..c66c25a --- /dev/null +++ b/tests/ClawdDotNet.Core.Tests/Files/FileLedgerTests.cs @@ -0,0 +1,188 @@ +using ClawdDotNet.Core.Audit; +using ClawdDotNet.Core.Files; +using ClawdDotNet.Core.Storage; +using ClawdDotNet.Core.Tests.Infrastructure; +using Shouldly; + +namespace ClawdDotNet.Core.Tests.Files; + +/// +/// #720: The lost update — A reads, B reads, A writes, B writes, and A's change is gone +/// without a word. The ledger refuses B's write instead and keeps the conflict for the count. +/// +public sealed class FileLedgerTests : IDisposable +{ + private readonly string _dir = Path.Combine(Path.GetTempPath(), "clawd-tests", "ledger-" + Guid.NewGuid().ToString("N")[..8]); + private readonly string _file; + private readonly FileLedger _ledger; + + public FileLedgerTests() + { + Directory.CreateDirectory(_dir); + _file = Path.Combine(_dir, "plan.md"); + File.WriteAllText(_file, "v1"); + _ledger = new FileLedger(new SqliteStorage(Path.Combine(_dir, "state.db"))); + } + + public void Dispose() + { + SqliteStorage.ReleasePooledConnections(_dir); + try { Directory.Delete(_dir, recursive: true); } + catch { /* cleanup is secondary */ } + } + + private string? Now => FileLedger.HashOf(_file); + + private void Write(string agent, string content) + { + File.WriteAllText(_file, content); + _ledger.NoteReplace(agent, "run-" + agent, _file, Now); + } + + [Fact] + public void The_second_writer_of_the_same_read_is_refused() + { + _ledger.NoteRead("a", _file, Now); + _ledger.NoteRead("b", _file, Now); + + _ledger.CheckReplace("a", "run-a", _file, "shared", Now).ShouldBeNull(); + Write("a", "v2 by a"); + + var conflict = _ledger.CheckReplace("b", "run-b", _file, "shared", Now); + conflict.ShouldNotBeNull(); + conflict.Kind.ShouldBe(FileConflict.ChangedSinceRead); + conflict.OtherAgentId.ShouldBe("a"); + conflict.OtherRunId.ShouldBe("run-a"); + } + + [Fact] + public void After_reading_again_the_write_goes_through() + { + _ledger.NoteRead("b", _file, Now); + Write("a", "v2 by a"); + _ledger.CheckReplace("b", null, _file, "shared", Now).ShouldNotBeNull(); + + _ledger.NoteRead("b", _file, Now); + + _ledger.CheckReplace("b", null, _file, "shared", Now).ShouldBeNull(); + } + + [Fact] + public void Writing_twice_in_a_row_is_not_a_conflict_with_oneself() + { + _ledger.NoteRead("a", _file, Now); + Write("a", "v2"); + + _ledger.CheckReplace("a", null, _file, "shared", Now).ShouldBeNull(); + } + + [Fact] + public void Another_agents_file_is_not_overwritten_unread() + { + Write("a", "a's work"); + + var conflict = _ledger.CheckReplace("b", null, _file, "shared", Now); + + conflict.ShouldNotBeNull(); + conflict.Kind.ShouldBe(FileConflict.NotRead); + conflict.Message("shared:/plan.md").ShouldContain("have not read it"); + } + + [Fact] + public void The_own_file_and_a_file_of_unknown_origin_may_be_overwritten_unread() + { + Write("a", "a's work"); + _ledger.CheckReplace("a", null, _file, "shared", Now).ShouldBeNull(); + + // A file nobody wrote through the ledger — from before, or by a human. + File.WriteAllText(_file, "put there by hand"); + _ledger.CheckReplace("b", null, _file, "shared", Now).ShouldBeNull(); + } + + [Fact] + public void A_new_file_replaces_nothing() + => _ledger.CheckReplace("b", null, Path.Combine(_dir, "new.md"), "shared", null).ShouldBeNull(); + + [Fact] + public void An_append_by_another_makes_an_older_read_stale() + { + _ledger.NoteRead("a", _file, Now); + + var before = Now; + File.AppendAllText(_file, "\n+ b's line"); + _ledger.NoteAppend("b", "run-b", _file, before, Now); + + var conflict = _ledger.CheckReplace("a", null, _file, "shared", Now); + conflict.ShouldNotBeNull("a's replace would drop b's appended line"); + conflict.OtherAgentId.ShouldBe("b"); + } + + [Fact] + public void An_own_append_on_a_known_file_keeps_the_read_valid() + { + _ledger.NoteRead("a", _file, Now); + var before = Now; + File.AppendAllText(_file, "\n+ a's line"); + _ledger.NoteAppend("a", null, _file, before, Now); + + _ledger.CheckReplace("a", null, _file, "shared", Now).ShouldBeNull(); + } + + [Fact] + public void A_change_by_hand_after_the_read_is_a_conflict_too() + { + _ledger.NoteRead("a", _file, Now); + File.WriteAllText(_file, "edited by a human"); + + var conflict = _ledger.CheckReplace("a", null, _file, "shared", Now); + + conflict.ShouldNotBeNull(); + conflict.OtherAgentId.ShouldBeNull(); + conflict.Message("shared:/plan.md").ShouldContain("someone else"); + } + + [Fact] + public async Task Hotspots_are_files_with_repeated_conflicts() + { + var other = Path.Combine(_dir, "once.md"); + for (var i = 0; i < 3; i++) + await _ledger.RecordAsync(new FileConflict(_file, "shared", "b", null, "a", null, FileConflict.ChangedSinceRead, DateTime.UtcNow), default); + await _ledger.RecordAsync(new FileConflict(other, "shared", "c", null, null, null, FileConflict.NotRead, DateTime.UtcNow), default); + await _ledger.RecordAsync(new FileConflict(_file, "shared", "b", null, "a", null, FileConflict.ChangedSinceRead, DateTime.UtcNow.AddDays(-30)), default); + + var hotspots = await _ledger.HotspotsAsync(DateTime.UtcNow.AddDays(-7), minimum: 3, limit: 10, default); + + hotspots.Count.ShouldBe(1); + hotspots[0].Path.ShouldBe(_file); + hotspots[0].Conflicts.ShouldBe(3, "the one from a month ago is outside the window"); + hotspots[0].Agents.ShouldBe(["a", "b"]); + } + + [Fact] + public async Task The_engine_audits_a_refused_write_as_a_conflict_not_an_error() + { + var audit = new InMemoryAuditRepository(); + var fixture = new EngineFixture(audit).WithTool(FakeTool.Conflicting("Conflict: plan.md was changed")); + var agent = fixture.AddAgent("agent-a", "TestTool"); + fixture.Client.RespondsWithToolCall("TestTool").RespondsWithText("done"); + + await fixture.Engine.RunAsync(agent, "go", "test-instance", default); + + audit.Entries.Single(e => e.Status != AuditStatus.Started).Status.ShouldBe(AuditStatus.Conflict); + } + + [Fact] + public async Task Tools_find_the_ledger_in_their_context() + { + Core.Tools.AgentToolContext? seen = null; + var fixture = new EngineFixture().WithTool(FakeTool.Observing(c => seen = c)); + fixture.Engine.Files = _ledger; + var agent = fixture.AddAgent("agent-a", "TestTool"); + fixture.Client.RespondsWithToolCall("TestTool").RespondsWithText("done"); + + await fixture.Engine.RunAsync(agent, "go", "test-instance", default); + + seen!.Files.ShouldBeSameAs(_ledger); + seen.RunId.ShouldNotBeNullOrWhiteSpace("a conflict names the run"); + } +} diff --git a/tests/ClawdDotNet.Core.Tests/Infrastructure/FakeTool.cs b/tests/ClawdDotNet.Core.Tests/Infrastructure/FakeTool.cs index bc4daa8..1114758 100644 --- a/tests/ClawdDotNet.Core.Tests/Infrastructure/FakeTool.cs +++ b/tests/ClawdDotNet.Core.Tests/Infrastructure/FakeTool.cs @@ -78,6 +78,14 @@ internal sealed class FakeTool : IAgentTool public static FakeTool Failing(string error, string name = "TestTool") => new(name, (_, _, _) => Task.FromResult(ToolResult.Fail(error))); + /// #720: Refuses with a write conflict. + public static FakeTool Conflicting(string error, string name = "TestTool") + => new(name, (_, _, _) => Task.FromResult(ToolResult.Conflict(error))); + + /// Hands the context it was called with to . + public static FakeTool Observing(Action seen, string name = "TestTool") + => new(name, (_, context, _) => { seen(context); return Task.FromResult(ToolResult.Ok("ok")); }); + public static FakeTool Throwing(Exception ex, string name = "TestTool") => new(name, (_, _, _) => throw ex); diff --git a/tests/ClawdDotNet.Tools.Tests/FileRW/LostUpdateTests.cs b/tests/ClawdDotNet.Tools.Tests/FileRW/LostUpdateTests.cs new file mode 100644 index 0000000..6fc5a86 --- /dev/null +++ b/tests/ClawdDotNet.Tools.Tests/FileRW/LostUpdateTests.cs @@ -0,0 +1,141 @@ +using System.Text.Json; +using ClawdDotNet.Core.Files; +using ClawdDotNet.Core.Tools; +using ClawdDotNet.Tools.FileRW; +using Microsoft.Extensions.Logging.Abstractions; +using Shouldly; + +namespace ClawdDotNet.Tools.Tests.FileRW; + +/// +/// #720: Two agents on the same file in the shared workspace. Nothing locks across steps; +/// the write that would lose the other's change is refused, and the agent reads again. +/// +public sealed class LostUpdateTests : IDisposable +{ + private readonly string _agents = Path.Combine(Path.GetTempPath(), "clawd-tests", Guid.NewGuid().ToString("N"), "Agents"); + private readonly FileRWTool _tool = new(); + private readonly FileLedger _ledger = new(); + + public LostUpdateTests() => Directory.CreateDirectory(Path.Combine(_agents, "SharedWorkspace")); + + public void Dispose() + { + try { Directory.Delete(Path.GetDirectoryName(_agents)!, recursive: true); } + catch { /* cleaning up is secondary */ } + } + + private string Shared(string file) => Path.Combine(_agents, "SharedWorkspace", file); + + private Task Run(string agentId, object input, bool withLedger = true) + { + var personal = Path.Combine(_agents, agentId, "Workspace"); + Directory.CreateDirectory(personal); + var context = new AgentToolContext(agentId, "inst", + new Dictionary { ["sharedAccessLevel"] = "ReadWrite" }, + new NullStateStore(), NullLogger.Instance, CancellationToken.None, + WorkspacePath: personal, SharedWorkspacePath: Path.Combine(_agents, "SharedWorkspace")) + { + Files = withLedger ? _ledger : null, + RunId = "run-" + agentId + }; + return _tool.ExecuteAsync(JsonSerializer.SerializeToElement(input), context, CancellationToken.None); + } + + private Task Read(string agent, string file = "plan.md") => Run(agent, new { action = "read", workspace = "shared", path = file }); + private Task Write(string agent, string content, string file = "plan.md") => Run(agent, new { action = "write", workspace = "shared", path = file, content }); + + [Fact] + public async Task The_second_write_of_the_same_read_is_refused_and_the_first_survives() + { + await Write("a", "v1"); + await Read("a"); + await Read("b"); + + (await Write("a", "v2 by a")).Success.ShouldBeTrue(); + var b = await Write("b", "v2 by b"); + + b.Success.ShouldBeFalse(); + b.IsConflict.ShouldBeTrue(); + b.ErrorMessage!.ShouldContain("changed by agent 'a' since you read it"); + File.ReadAllText(Shared("plan.md")).ShouldBe("v2 by a"); + } + + [Fact] + public async Task Reading_again_resolves_it() + { + await Write("a", "v1"); + await Read("b"); + await Write("a", "v2 by a"); + (await Write("b", "v2 by b")).IsConflict.ShouldBeTrue(); + + (await Read("b")).Content.ShouldContain("v2 by a"); + (await Write("b", "v3 by b on top of a")).Success.ShouldBeTrue(); + } + + [Fact] + public async Task Another_agents_file_is_not_overwritten_blind_but_the_own_is() + { + await Write("a", "a's report"); + + (await Write("a", "a's report, revised")).Success.ShouldBeTrue("the own file may be overwritten without reading"); + var b = await Write("b", "b's version"); + + b.IsConflict.ShouldBeTrue(); + b.ErrorMessage!.ShouldContain("have not read it"); + } + + [Fact] + public async Task Appending_is_never_refused_but_makes_older_reads_stale() + { + await Write("a", "log"); + await Read("a"); + + (await Run("b", new { action = "append", workspace = "shared", path = "plan.md", content = "\nb was here" })).Success.ShouldBeTrue(); + var a = await Write("a", "log, rewritten"); + + a.IsConflict.ShouldBeTrue("the replace would drop b's line"); + File.ReadAllText(Shared("plan.md")).ShouldContain("b was here"); + } + + [Fact] + public async Task Copying_over_a_changed_file_is_a_write_like_any_other() + { + await Write("a", "v1"); + await Read("b"); + await Write("a", "v2 by a"); + await Run("b", new { action = "write", workspace = "shared", path = "draft.md", content = "b's draft" }); + + var copy = await Run("b", new { action = "copy", workspace = "shared", path = "draft.md", destinationPath = "plan.md" }); + + copy.IsConflict.ShouldBeTrue(); + File.ReadAllText(Shared("plan.md")).ShouldBe("v2 by a"); + } + + [Fact] + public async Task The_personal_workspace_is_not_checked() + { + await Run("a", new { action = "write", path = "mine.md", content = "v1" }); + await Run("a", new { action = "read", path = "mine.md" }); + File.WriteAllText(Path.Combine(_agents, "a", "Workspace", "mine.md"), "changed outside"); + + (await Run("a", new { action = "write", path = "mine.md", content = "v2" })).Success.ShouldBeTrue(); + } + + [Fact] + public async Task Without_the_ledger_everything_is_as_before() + { + await Write("a", "v1"); + (await Run("b", new { action = "write", workspace = "shared", path = "plan.md", content = "b" }, withLedger: false)).Success.ShouldBeTrue(); + } + + [Fact] + public async Task Reading_many_counts_as_reading_each() + { + await Write("a", "one", "x/1.md"); + await Write("a", "two", "x/2.md"); + await Run("b", new { action = "read_many", workspace = "shared", path = "x" }); + + (await Write("b", "one by b", "x/1.md")).Success.ShouldBeTrue(); + } +}