diff --git a/src/ClaudeDo.Worker/External/HandoffMcpTools.cs b/src/ClaudeDo.Worker/External/HandoffMcpTools.cs index 6f5fada7..b069500a 100644 --- a/src/ClaudeDo.Worker/External/HandoffMcpTools.cs +++ b/src/ClaudeDo.Worker/External/HandoffMcpTools.cs @@ -6,18 +6,23 @@ using ModelContextProtocol.Server; namespace ClaudeDo.Worker.External; -public sealed record HandoffListHandlerResult(bool Requested, string TaskId, int Number, int SurvivingCount, string NextPhase); +// Coerced=true means nextPhase asked for a non-final phase but the server-side round cap +// (HandoffRoundTracker) forced it to its "_final" counterpart instead -- NextPhase already +// reflects the effective (post-coercion) value in that case. +public sealed record HandoffListHandlerResult(bool Requested, string TaskId, int Number, int SurvivingCount, string NextPhase, bool Coerced = false); [McpServerToolType] public sealed class HandoffMcpTools { private readonly TaskRepository _tasks; private readonly HubBroadcaster _broadcaster; + private readonly HandoffRoundTracker _rounds; - public HandoffMcpTools(TaskRepository tasks, HubBroadcaster broadcaster) + public HandoffMcpTools(TaskRepository tasks, HubBroadcaster broadcaster, HandoffRoundTracker rounds) { _tasks = tasks; _broadcaster = broadcaster; + _rounds = rounds; } [McpServerTool, Description( @@ -27,7 +32,11 @@ public sealed class HandoffMcpTools "untouched. nextPhase picks the next session's role: \"wait\" (queue + wait; the normal handoff " + "after triage) or \"merge\" (review + merge; the normal handoff after wait) -- \"wait_final\"/" + "\"merge_final\" are only for a merge session that started reruns and must eventually stop the " + - "chain instead of restarting again. An unrecognized value is rejected. The current tile stays " + + "chain instead of restarting again. An unrecognized value is rejected. This handoff chain is " + + "capped server-side at " + nameof(HandoffRoundTracker.MaxHandoffs) + " (four handoffs, i.e. two " + + "full wait/merge cycles) for a given handler task: once reached, a \"wait\"/\"merge\" request is " + + "silently coerced to its \"_final\" counterpart instead (check the result's coerced field), and " + + "any handoff at all after a merge_final round for this task throws. The current tile stays " + "open; you must end your own turn immediately after calling this.")] public async Task HandoffListHandler( [Description("This session's own handler task id.")] string taskId, @@ -50,7 +59,11 @@ public sealed class HandoffMcpTools _ = await _tasks.GetByIdAsync(id, cancellationToken) ?? throw new InvalidOperationException($"Task {id} not found."); - await _broadcaster.HandoffRequested(taskId, survivingTaskIds, nextPhase); - return new HandoffListHandlerResult(true, taskId, task.Number, survivingTaskIds.Count, nextPhase); + var round = _rounds.Register(taskId, nextPhase); + if (round.Rejected) + throw new InvalidOperationException(round.Error); + + await _broadcaster.HandoffRequested(taskId, survivingTaskIds, round.EffectivePhase); + return new HandoffListHandlerResult(true, taskId, task.Number, survivingTaskIds.Count, round.EffectivePhase, round.Coerced); } } diff --git a/src/ClaudeDo.Worker/External/HandoffRoundTracker.cs b/src/ClaudeDo.Worker/External/HandoffRoundTracker.cs new file mode 100644 index 00000000..56516242 --- /dev/null +++ b/src/ClaudeDo.Worker/External/HandoffRoundTracker.cs @@ -0,0 +1,54 @@ +using System.Collections.Concurrent; +using ClaudeDo.Worker.Runner; + +namespace ClaudeDo.Worker.External; + +// Bounds the list-handler wait<->merge handoff chain server-side, so a model that ignores the +// cooperative "this is the FINAL round" line in handoff.md (InteractiveLaunchSpecService's +// finalNote) cannot loop wait->merge->wait->merge forever, each round spawning a fresh ConPTY +// session. Keyed by handler task id -- a "Let Claude handle it" run always creates a fresh +// handler task (InteractiveLaunchSpecService.CreateMergeHelperTaskAsync), so the counter is +// per-run by construction: no reset logic needed, and a later run on the same list starts at 0 +// on its own. In-memory only, same tradeoff as TaskRunTokenRegistry -- a worker restart mid-run +// resets the count, which is acceptable since the chain restarts fresh anyway. +public sealed class HandoffRoundTracker +{ + // Two full wait->merge cycles: wait, merge, wait, merge. + public const int MaxHandoffs = 4; + + private sealed class TaskState + { + public int Count; + public bool Sealed; + } + + private readonly ConcurrentDictionary _states = new(); + + public sealed record Result(bool Rejected, bool Coerced, string EffectivePhase, string? Error); + + public Result Register(string taskId, string nextPhase) + { + var state = _states.GetOrAdd(taskId, _ => new TaskState()); + lock (state) + { + if (state.Sealed) + return new Result(true, false, nextPhase, + $"Task {taskId} already completed its merge_final round -- no further handoff is allowed."); + + state.Count++; + var isFinal = nextPhase is MergeHelperPhase.WaitFinal or MergeHelperPhase.MergeFinal; + + var effective = nextPhase; + if (state.Count > MaxHandoffs && !isFinal) + { + effective = nextPhase == MergeHelperPhase.Wait ? MergeHelperPhase.WaitFinal : MergeHelperPhase.MergeFinal; + isFinal = true; + } + + if (isFinal && effective == MergeHelperPhase.MergeFinal) + state.Sealed = true; + + return new Result(false, !string.Equals(effective, nextPhase, StringComparison.Ordinal), effective, null); + } + } +} diff --git a/src/ClaudeDo.Worker/Program.cs b/src/ClaudeDo.Worker/Program.cs index 43a3b94d..ff7f7c8a 100644 --- a/src/ClaudeDo.Worker/Program.cs +++ b/src/ClaudeDo.Worker/Program.cs @@ -317,6 +317,7 @@ if (cfg.ExternalMcpPort > 0) externalBuilder.Services.AddScoped(); externalBuilder.Services.AddScoped(); externalBuilder.Services.AddScoped(); + externalBuilder.Services.AddSingleton(); externalBuilder.Services.AddScoped(); externalBuilder.Services.AddScoped(); externalBuilder.Services.AddScoped(); diff --git a/tests/ClaudeDo.Worker.Tests/External/HandoffMcpToolsTests.cs b/tests/ClaudeDo.Worker.Tests/External/HandoffMcpToolsTests.cs index 6309c6db..f7f6450d 100644 --- a/tests/ClaudeDo.Worker.Tests/External/HandoffMcpToolsTests.cs +++ b/tests/ClaudeDo.Worker.Tests/External/HandoffMcpToolsTests.cs @@ -25,7 +25,8 @@ public sealed class HandoffMcpToolsTests : IDisposable public void Dispose() { _ctx.Dispose(); _db.Dispose(); } - private HandoffMcpTools BuildSut() => new(_tasks, new HubBroadcaster(_hubContext)); + private HandoffMcpTools BuildSut(HandoffRoundTracker? tracker = null) => + new(_tasks, new HubBroadcaster(_hubContext), tracker ?? new HandoffRoundTracker()); private async Task SeedTaskAsync(string listId, TaskStatus status = TaskStatus.Idle, string title = "t") { @@ -138,4 +139,85 @@ public sealed class HandoffMcpToolsTests : IDisposable await Assert.ThrowsAsync(() => sut.HandoffListHandler(handlerTask.Id, new[] { "missing" }, cancellationToken: CancellationToken.None)); } + + [Fact] + public async Task HandoffListHandler_NormalWaitMergeWaitFinalMergeFinalSequence_PassesUntouched() + { + var listId = await SeedListAsync(); + var handlerTask = await SeedTaskAsync(listId); + var survivor = await SeedTaskAsync(listId, TaskStatus.WaitingForReview, title: "Survivor"); + var tracker = new HandoffRoundTracker(); + var survivors = new[] { survivor.Id }; + + foreach (var phase in new[] { "wait", "merge", "wait_final", "merge_final" }) + { + var sut = BuildSut(tracker); + var result = await sut.HandoffListHandler(handlerTask.Id, survivors, phase, CancellationToken.None); + Assert.False(result.Coerced); + Assert.Equal(phase, result.NextPhase); + } + } + + [Fact] + public async Task HandoffListHandler_PastCap_CoercesNonFinalPhaseToItsFinalCounterpart() + { + var listId = await SeedListAsync(); + var handlerTask = await SeedTaskAsync(listId); + var survivor = await SeedTaskAsync(listId, TaskStatus.WaitingForReview, title: "Survivor"); + var tracker = new HandoffRoundTracker(); + var survivors = new[] { survivor.Id }; + + foreach (var phase in new[] { "wait", "merge", "wait", "merge" }) + { + var sut = BuildSut(tracker); + var result = await sut.HandoffListHandler(handlerTask.Id, survivors, phase, CancellationToken.None); + Assert.False(result.Coerced); + } + + var fifthSut = BuildSut(tracker); + var fifth = await fifthSut.HandoffListHandler(handlerTask.Id, survivors, "wait", CancellationToken.None); + + Assert.True(fifth.Coerced); + Assert.Equal("wait_final", fifth.NextPhase); + } + + [Fact] + public async Task HandoffListHandler_AfterMergeFinal_AnyFurtherHandoffThrows() + { + var listId = await SeedListAsync(); + var handlerTask = await SeedTaskAsync(listId); + var survivor = await SeedTaskAsync(listId, TaskStatus.WaitingForReview, title: "Survivor"); + var tracker = new HandoffRoundTracker(); + var survivors = new[] { survivor.Id }; + + var firstSut = BuildSut(tracker); + await firstSut.HandoffListHandler(handlerTask.Id, survivors, "merge_final", CancellationToken.None); + + var secondSut = BuildSut(tracker); + await Assert.ThrowsAsync(() => + secondSut.HandoffListHandler(handlerTask.Id, survivors, "wait", CancellationToken.None)); + } + + [Fact] + public async Task HandoffListHandler_TwoHandlerTasks_DoNotShareACounter() + { + var listId = await SeedListAsync(); + var handlerA = await SeedTaskAsync(listId, title: "Handler A"); + var handlerB = await SeedTaskAsync(listId, title: "Handler B"); + var survivorA = await SeedTaskAsync(listId, TaskStatus.WaitingForReview, title: "Survivor A"); + var survivorB = await SeedTaskAsync(listId, TaskStatus.WaitingForReview, title: "Survivor B"); + var tracker = new HandoffRoundTracker(); + + foreach (var phase in new[] { "wait", "merge", "wait", "merge" }) + { + var sut = BuildSut(tracker); + await sut.HandoffListHandler(handlerA.Id, new[] { survivorA.Id }, phase, CancellationToken.None); + } + + var sutB = BuildSut(tracker); + var resultB = await sutB.HandoffListHandler(handlerB.Id, new[] { survivorB.Id }, "wait", CancellationToken.None); + + Assert.False(resultB.Coerced); + Assert.Equal("wait", resultB.NextPhase); + } }