From 11103d4d3e874eab278475092870422a5fdcae7a Mon Sep 17 00:00:00 2001 From: mika kuns Date: Tue, 11 Aug 2026 08:32:53 +0200 Subject: [PATCH] fix(runner): only fail a run on denied write tools with a worktree --- src/ClaudeDo.Worker/Runner/TaskRunner.cs | 22 ++++++-- .../Runner/PermissionDenialFailureTests.cs | 54 +++++++++++++++++++ 2 files changed, 72 insertions(+), 4 deletions(-) diff --git a/src/ClaudeDo.Worker/Runner/TaskRunner.cs b/src/ClaudeDo.Worker/Runner/TaskRunner.cs index 4ad95b6b..376d541d 100644 --- a/src/ClaudeDo.Worker/Runner/TaskRunner.cs +++ b/src/ClaudeDo.Worker/Runner/TaskRunner.cs @@ -496,13 +496,18 @@ public sealed class TaskRunner // downgrade to interactive "default" (see PermissionModeResolver). Left alone this // lands as a normal WaitingForReview with an empty diff, and a reviewer sees only // that emptiness with no clue why. Surface it as a failure instead. - if (!committed && result.PermissionDenials.Count > 0) + // + // Narrow on purpose. It only fires when a worktree exists (a sandbox run has no diff + // whose emptiness could mean anything) and only for denied WRITE tools: a research task + // that legitimately changes nothing, or one that had a single Bash call denied, is a + // successful run and must not be reported as "all edits blocked". + var deniedWrites = result.PermissionDenials.Where(IsWriteTool).Distinct().ToList(); + if (wtCtx is not null && !committed && deniedWrites.Count > 0) { - var tools = string.Join(", ", result.PermissionDenials.Distinct()); await MarkFailed( task.Id, task.Title, slot, - $"All edits were blocked by permission denials ({tools}) and nothing was changed. " + - "Check the run's permission mode (get_effective_run_config).", + $"All edits were blocked by permission denials ({string.Join(", ", deniedWrites)}) and nothing " + + "was changed. Check the run's permission mode (get_effective_run_config).", result.TurnCount); return; } @@ -578,6 +583,15 @@ public sealed class TaskRunner } } + // Tools whose denial means the run could not change files. Everything else (Bash, WebFetch, + // an MCP tool, …) can be denied in a perfectly successful run, so it must not mark one failed. + private static readonly HashSet WriteTools = new(StringComparer.OrdinalIgnoreCase) + { + "Edit", "Write", "MultiEdit", "NotebookEdit", "Update", + }; + + internal static bool IsWriteTool(string toolName) => WriteTools.Contains(toolName); + /// Classifies the CLI's raw terminal_reason into the small, MCP-facing enum /// (max_turns|timeout|error) get_task/batch_get_tasks report as failureReason. /// "cancelled" is set explicitly at the call sites that know it (there's no CLI signal for it). diff --git a/tests/ClaudeDo.Worker.Tests/Runner/PermissionDenialFailureTests.cs b/tests/ClaudeDo.Worker.Tests/Runner/PermissionDenialFailureTests.cs index 8ce0abe7..17a30fe6 100644 --- a/tests/ClaudeDo.Worker.Tests/Runner/PermissionDenialFailureTests.cs +++ b/tests/ClaudeDo.Worker.Tests/Runner/PermissionDenialFailureTests.cs @@ -134,6 +134,60 @@ public sealed class PermissionDenialFailureTests : IDisposable } } + [Fact] + public async Task Success_withNonWriteDenial_andNoChanges_stillWaitsForReview() + { + if (!GitRepoFixture.IsGitAvailable()) { Assert.True(true, "git not available -- skipping"); return; } + var repo = new GitRepoFixture(); _repos.Add(repo); + var db = new DbFixture(); _dbs.Add(db); + var dbFactory = db.CreateFactory(); + var tempDir = Path.Combine(Path.GetTempPath(), $"cd_permdenial_{Guid.NewGuid():N}"); + Directory.CreateDirectory(tempDir); + var cfg = new WorkerConfig { SandboxRoot = tempDir, LogRoot = tempDir, WorktreeRootStrategy = "sibling" }; + + using (var ctx = db.CreateContext()) + { + ctx.Lists.Add(new ListEntity { Id = "l1", Name = "L", WorkingDir = repo.RepoDir, CreatedAt = DateTime.UtcNow }); + ctx.Tasks.Add(new TaskEntity + { + Id = "t1", ListId = "l1", Title = "Investigate something", CommitType = "chore", + Status = TaskStatus.Idle, CreatedAt = DateTime.UtcNow, + }); + await ctx.SaveChangesAsync(); + } + + // A research task that legitimately changes nothing and had one non-write tool denied. + // "no commit + some denial" must NOT be read as "all edits were blocked". + var fake = new FakeClaudeProcess((_, _, _, _, _) => Task.FromResult(new RunResult + { + ExitCode = 0, + ResultMarkdown = "Investigated; no changes needed. One command was not permitted.", + PermissionDenials = new[] { "Bash" }, + })); + var state = TaskStateServiceBuilder.Build(dbFactory).State; + var wt = new WorktreeManager(new GitService(), dbFactory, cfg, NullLogger.Instance); + var runner = new TaskRunner(fake, dbFactory, new HubBroadcaster(new CapturingHubContext()), wt, + new ClaudeArgsBuilder(), cfg, NullLogger.Instance, state, new TaskRunTokenRegistry(), + new AttachmentStore(), new FakeSessionSkillSeeder(), new FakeTranscriptUsageReader()); + + try + { + using (var ctx = db.CreateContext()) + await runner.RunAsync((await new TaskRepository(ctx).GetByIdAsync("t1"))!, "slot-1", CancellationToken.None); + + using var verify = db.CreateContext(); + var task = (await new TaskRepository(verify).GetByIdAsync("t1"))!; + Assert.Equal(TaskStatus.WaitingForReview, task.Status); + } + finally + { + using var ctx = db.CreateContext(); + var wtRow = await new WorktreeRepository(ctx).GetByTaskIdAsync("t1"); + if (wtRow is not null) _cleanups.Add((repo.RepoDir, wtRow.Path)); + try { Directory.Delete(tempDir, true); } catch { } + } + } + public void Dispose() { foreach (var (repoDir, wtPath) in _cleanups)