diff --git a/src/ClaudeDo.Worker/Runner/ClaudeArgsBuilder.cs b/src/ClaudeDo.Worker/Runner/ClaudeArgsBuilder.cs index 56a783f6..bd39ddc1 100644 --- a/src/ClaudeDo.Worker/Runner/ClaudeArgsBuilder.cs +++ b/src/ClaudeDo.Worker/Runner/ClaudeArgsBuilder.cs @@ -41,12 +41,8 @@ public sealed class ClaudeArgsBuilder "--verbose", }; - var permissionMode = string.IsNullOrWhiteSpace(config.PermissionMode) - || config.PermissionMode.Equals("bypassPermissions", StringComparison.OrdinalIgnoreCase) - ? "auto" - : config.PermissionMode; args.Add("--permission-mode"); - args.Add(permissionMode); + args.Add(PermissionModeResolver.Resolve(config.Model, config.PermissionMode)); if (config.Model is not null) { diff --git a/src/ClaudeDo.Worker/Runner/ClaudeProcess.cs b/src/ClaudeDo.Worker/Runner/ClaudeProcess.cs index 84f43c57..a9bad3a7 100644 --- a/src/ClaudeDo.Worker/Runner/ClaudeProcess.cs +++ b/src/ClaudeDo.Worker/Runner/ClaudeProcess.cs @@ -127,6 +127,7 @@ public sealed class ClaudeProcess : IClaudeProcess ResultSubtype = streamResult.ResultSubtype, TerminalReason = streamResult.TerminalReason, Errors = streamResult.Errors, + PermissionDenials = streamResult.PermissionDenials, }; } @@ -150,6 +151,7 @@ public sealed class ClaudeProcess : IClaudeProcess ResultSubtype = streamResult.ResultSubtype, TerminalReason = streamResult.TerminalReason, Errors = streamResult.Errors, + PermissionDenials = streamResult.PermissionDenials, }; } } diff --git a/src/ClaudeDo.Worker/Runner/EffectiveRunConfigResolver.cs b/src/ClaudeDo.Worker/Runner/EffectiveRunConfigResolver.cs index 8936ed13..ea0bad42 100644 --- a/src/ClaudeDo.Worker/Runner/EffectiveRunConfigResolver.cs +++ b/src/ClaudeDo.Worker/Runner/EffectiveRunConfigResolver.cs @@ -47,7 +47,7 @@ public static class EffectiveRunConfigResolver maxTurns, maxTurnsSource, requestedMaxTurns, maxTurns < requestedMaxTurns, preset.Effort, agentPath, agentPathSource, - global.DefaultPermissionMode, + PermissionModeResolver.Resolve(model, global.DefaultPermissionMode), systemPromptSources.Count > 0, systemPromptSources); } } diff --git a/src/ClaudeDo.Worker/Runner/PermissionModeResolver.cs b/src/ClaudeDo.Worker/Runner/PermissionModeResolver.cs new file mode 100644 index 00000000..9c764c99 --- /dev/null +++ b/src/ClaudeDo.Worker/Runner/PermissionModeResolver.cs @@ -0,0 +1,28 @@ +using ClaudeDo.Data.Models; + +namespace ClaudeDo.Worker.Runner; + +/// Single source of truth for the permission mode actually started, shared by +/// 's real dispatch and 's +/// read-only report — so the two can never drift apart. +public static class PermissionModeResolver +{ + public static string Resolve(string? model, string? requestedPermissionMode) + { + var mode = string.IsNullOrWhiteSpace(requestedPermissionMode) + || requestedPermissionMode.Equals("bypassPermissions", StringComparison.OrdinalIgnoreCase) + ? "auto" + : requestedPermissionMode; + + // claude-cli 2.1.220 silently downgrades "--permission-mode auto" to the interactive + // "default" mode when --model resolves to a haiku model (confirmed by capturing the + // stream-json `init` event for both models side by side; acceptEdits/bypassPermissions + // pass through unaffected). An unattended run then blocks forever on an edit confirmation + // that never arrives, so substitute the closest unattended-safe mode instead. + if (string.Equals(mode, "auto", StringComparison.OrdinalIgnoreCase) + && ModelRegistry.TryNormalizeAlias(model) == "haiku") + return "acceptEdits"; + + return mode; + } +} diff --git a/src/ClaudeDo.Worker/Runner/RunResult.cs b/src/ClaudeDo.Worker/Runner/RunResult.cs index 3b26e527..5502a9f0 100644 --- a/src/ClaudeDo.Worker/Runner/RunResult.cs +++ b/src/ClaudeDo.Worker/Runner/RunResult.cs @@ -14,6 +14,7 @@ public sealed record RunResult public string? ResultSubtype { get; init; } public string? TerminalReason { get; init; } public IReadOnlyList Errors { get; init; } = Array.Empty(); + public IReadOnlyList PermissionDenials { get; init; } = Array.Empty(); public bool IsSuccess => ExitCode == 0 && ResultMarkdown is not null; } diff --git a/src/ClaudeDo.Worker/Runner/StreamAnalyzer.cs b/src/ClaudeDo.Worker/Runner/StreamAnalyzer.cs index 4ee65552..78cacdda 100644 --- a/src/ClaudeDo.Worker/Runner/StreamAnalyzer.cs +++ b/src/ClaudeDo.Worker/Runner/StreamAnalyzer.cs @@ -16,6 +16,7 @@ public sealed class StreamResult public string? ResultSubtype { get; set; } public string? TerminalReason { get; set; } public IReadOnlyList Errors { get; set; } = Array.Empty(); + public IReadOnlyList PermissionDenials { get; set; } = Array.Empty(); } public sealed class StreamAnalyzer @@ -31,6 +32,7 @@ public sealed class StreamAnalyzer private string? _resultSubtype; private string? _terminalReason; private readonly List _errors = new(); + private readonly List _permissionDenials = new(); private const string BlockedPrefix = "CLAUDEDO_BLOCKED:"; public void ProcessLine(string ndjsonLine) @@ -65,6 +67,17 @@ public sealed class StreamAnalyzer && !string.IsNullOrEmpty(errorText)) _errors.Add(errorText); } + // A CLI-level permission-mode failure (e.g. haiku silently downgrading + // "auto" to the interactive "default") still reports is_error:false — the + // denials only show up here, on the result event. + if (root.TryGetProperty("permission_denials", out var denialsProp) + && denialsProp.ValueKind == JsonValueKind.Array) + { + foreach (var denial in denialsProp.EnumerateArray()) + if (denial.TryGetProperty("tool_name", out var toolNameProp) + && toolNameProp.GetString() is { } toolName && !string.IsNullOrEmpty(toolName)) + _permissionDenials.Add(toolName); + } // Authoritative token totals live on the result event. if (root.TryGetProperty("usage", out var resultUsage)) { @@ -107,6 +120,7 @@ public sealed class StreamAnalyzer ResultSubtype = _resultSubtype, TerminalReason = _terminalReason, Errors = _errors, + PermissionDenials = _permissionDenials, }; private string? FallbackResult() diff --git a/src/ClaudeDo.Worker/Runner/TaskRunner.cs b/src/ClaudeDo.Worker/Runner/TaskRunner.cs index e4af0422..1c8196fa 100644 --- a/src/ClaudeDo.Worker/Runner/TaskRunner.cs +++ b/src/ClaudeDo.Worker/Runner/TaskRunner.cs @@ -457,9 +457,10 @@ public sealed class TaskRunner private async Task HandleSuccess(TaskEntity task, ListEntity list, string slot, WorktreeContext? wtCtx, RunResult result, CancellationToken ct) { + var committed = false; if (wtCtx is not null) { - var committed = await _wtManager.CommitIfChangedAsync(wtCtx, task, list, ct); + committed = await _wtManager.CommitIfChangedAsync(wtCtx, task, list, ct); if (committed) { await _broadcaster.WorkerLog($"Committed changes in \"{task.Title}\"", WorkerLogLevel.Info, DateTime.UtcNow); @@ -467,6 +468,22 @@ public sealed class TaskRunner } } + // A run can report success (exit 0, non-null result text) while every write it + // attempted was denied by the permission gate — e.g. the claude-cli haiku/"auto" + // 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) + { + 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).", + result.TurnCount); + return; + } + // Terminal DB write uses CancellationToken.None so the task status // is never left as 'running' because of a cancel that arrived // after the Claude run already succeeded. diff --git a/tests/ClaudeDo.Worker.Tests/Runner/ClaudeArgsBuilderTests.cs b/tests/ClaudeDo.Worker.Tests/Runner/ClaudeArgsBuilderTests.cs index 7158a203..efdc0c03 100644 --- a/tests/ClaudeDo.Worker.Tests/Runner/ClaudeArgsBuilderTests.cs +++ b/tests/ClaudeDo.Worker.Tests/Runner/ClaudeArgsBuilderTests.cs @@ -170,6 +170,29 @@ public sealed class ClaudeArgsBuilderTests Assert.DoesNotContain("--dangerously-skip-permissions", args); } + [Theory] + [InlineData("haiku", "auto", "acceptEdits")] + [InlineData("haiku", null, "acceptEdits")] + [InlineData("haiku", "bypassPermissions", "acceptEdits")] + [InlineData("claude-haiku-4-5-20251001", "auto", "acceptEdits")] + [InlineData("haiku", "acceptEdits", "acceptEdits")] + [InlineData("haiku", "plan", "plan")] + [InlineData("sonnet", "auto", "auto")] + [InlineData("claude-sonnet-4-6", "auto", "auto")] + [InlineData(null, "auto", "auto")] + public void PermissionMode_ForHaikuModel_SubstitutesAutoWithAcceptEdits( + string? model, string? requestedMode, string expectedMode) + { + // claude-cli 2.1.220 silently downgrades "--permission-mode auto" to the interactive + // "default" mode when the resolved model is a haiku model (verified against the real + // CLI's stream-json `init` event), which would block an unattended run forever. + var args = _builder.Build(new ClaudeRunConfig(model, null, null, null, PermissionMode: requestedMode)); + var list = args.ToList(); + var idx = list.IndexOf("--permission-mode"); + Assert.True(idx >= 0); + Assert.Equal(expectedMode, list[idx + 1]); + } + [Fact] public void Build_emits_mcpConfig_and_allowedTools_when_set() { diff --git a/tests/ClaudeDo.Worker.Tests/Runner/PermissionDenialFailureTests.cs b/tests/ClaudeDo.Worker.Tests/Runner/PermissionDenialFailureTests.cs new file mode 100644 index 00000000..8ce0abe7 --- /dev/null +++ b/tests/ClaudeDo.Worker.Tests/Runner/PermissionDenialFailureTests.cs @@ -0,0 +1,144 @@ +using ClaudeDo.Data; +using ClaudeDo.Data.Git; +using ClaudeDo.Data.Models; +using ClaudeDo.Data.Repositories; +using ClaudeDo.Worker.Config; +using ClaudeDo.Worker.Hub; +using ClaudeDo.Worker.Runner; +using ClaudeDo.Worker.Tests.Infrastructure; +using Microsoft.Extensions.Logging.Abstractions; +using TaskStatus = ClaudeDo.Data.Models.TaskStatus; +using Xunit; + +namespace ClaudeDo.Worker.Tests.Runner; + +/// +/// A run that reports success (exit 0, non-null result text) but whose only tool calls were +/// permission-denied — e.g. the claude-cli haiku/"auto" downgrade to interactive "default" — must +/// not land as a normal WaitingForReview with an empty diff. See PermissionModeResolver. +/// +public sealed class PermissionDenialFailureTests : IDisposable +{ + private readonly List _repos = new(); + private readonly List _dbs = new(); + private readonly List<(string repoDir, string wtPath)> _cleanups = new(); + + [Fact] + public async Task Success_withPermissionDenials_andNoChanges_endsFailed() + { + 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 = "Edit a file", CommitType = "chore", + Status = TaskStatus.Idle, CreatedAt = DateTime.UtcNow, + }); + await ctx.SaveChangesAsync(); + } + + // The Edit tool was denied; no file changed. Mirrors the real "result" event's shape. + var fake = new FakeClaudeProcess((_, _, _, _, _) => Task.FromResult(new RunResult + { + ExitCode = 0, + ResultMarkdown = "I need permission to edit the file.", + PermissionDenials = new[] { "Edit" }, + })); + 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.Failed, task.Status); + Assert.Contains("permission", task.Result, StringComparison.OrdinalIgnoreCase); + } + 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 { } + } + } + + [Fact] + public async Task Success_withPermissionDenials_butSomeChangesCommitted_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 = "Edit some files", CommitType = "chore", + Status = TaskStatus.Idle, CreatedAt = DateTime.UtcNow, + }); + await ctx.SaveChangesAsync(); + } + + // One edit succeeded (a file was actually written), another was denied. + var fake = new FakeClaudeProcess((_, workingDirectory, _, _, _) => + { + File.WriteAllText(Path.Combine(workingDirectory, "changed.txt"), "partial success"); + return Task.FromResult(new RunResult + { + ExitCode = 0, + ResultMarkdown = "Made one change; one edit was denied.", + PermissionDenials = new[] { "Write" }, + }); + }); + 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) + try { GitRepoFixture.RunGit(repoDir, "worktree", "remove", "--force", wtPath); } catch { } + foreach (var r in _repos) r.Dispose(); + foreach (var d in _dbs) d.Dispose(); + } +} diff --git a/tests/ClaudeDo.Worker.Tests/Runner/StreamAnalyzerTests.cs b/tests/ClaudeDo.Worker.Tests/Runner/StreamAnalyzerTests.cs index 4e164a6e..ac82f29e 100644 --- a/tests/ClaudeDo.Worker.Tests/Runner/StreamAnalyzerTests.cs +++ b/tests/ClaudeDo.Worker.Tests/Runner/StreamAnalyzerTests.cs @@ -186,6 +186,41 @@ public sealed class StreamAnalyzerTests Assert.Empty(result.Errors); } + [Fact] + public void Extracts_Permission_Denials_From_Result_Event() + { + var analyzer = new StreamAnalyzer(); + analyzer.ProcessLine(""" + {"type":"result","result":"I need permission to edit the file.","session_id":"s1", + "permission_denials":[{"tool_name":"Edit","tool_use_id":"t1","tool_input":{}}]} + """); + var result = analyzer.GetResult(); + Assert.Single(result.PermissionDenials); + Assert.Equal("Edit", result.PermissionDenials[0]); + } + + [Fact] + public void No_Permission_Denials_Field_Means_Empty_List() + { + var analyzer = new StreamAnalyzer(); + analyzer.ProcessLine("""{"type":"result","result":"done","session_id":"s1"}"""); + Assert.Empty(analyzer.GetResult().PermissionDenials); + } + + [Fact] + public void Multiple_Permission_Denials_Are_All_Collected() + { + var analyzer = new StreamAnalyzer(); + analyzer.ProcessLine(""" + {"type":"result","result":"blocked","session_id":"s1","permission_denials":[ + {"tool_name":"Edit","tool_use_id":"t1","tool_input":{}}, + {"tool_name":"Write","tool_use_id":"t2","tool_input":{}} + ]} + """); + var result = analyzer.GetResult(); + Assert.Equal(new[] { "Edit", "Write" }, result.PermissionDenials); + } + [Fact] public void Duplicate_Marker_In_Assistant_And_Result_Is_Collected_Once() {