fix(runner): only fail a run on denied write tools with a worktree
This commit is contained in:
@@ -496,13 +496,18 @@ public sealed class TaskRunner
|
|||||||
// downgrade to interactive "default" (see PermissionModeResolver). Left alone this
|
// downgrade to interactive "default" (see PermissionModeResolver). Left alone this
|
||||||
// lands as a normal WaitingForReview with an empty diff, and a reviewer sees only
|
// 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.
|
// 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(
|
await MarkFailed(
|
||||||
task.Id, task.Title, slot,
|
task.Id, task.Title, slot,
|
||||||
$"All edits were blocked by permission denials ({tools}) and nothing was changed. " +
|
$"All edits were blocked by permission denials ({string.Join(", ", deniedWrites)}) and nothing " +
|
||||||
"Check the run's permission mode (get_effective_run_config).",
|
"was changed. Check the run's permission mode (get_effective_run_config).",
|
||||||
result.TurnCount);
|
result.TurnCount);
|
||||||
return;
|
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<string> WriteTools = new(StringComparer.OrdinalIgnoreCase)
|
||||||
|
{
|
||||||
|
"Edit", "Write", "MultiEdit", "NotebookEdit", "Update",
|
||||||
|
};
|
||||||
|
|
||||||
|
internal static bool IsWriteTool(string toolName) => WriteTools.Contains(toolName);
|
||||||
|
|
||||||
/// <summary>Classifies the CLI's raw <c>terminal_reason</c> into the small, MCP-facing enum
|
/// <summary>Classifies the CLI's raw <c>terminal_reason</c> into the small, MCP-facing enum
|
||||||
/// (<c>max_turns|timeout|error</c>) get_task/batch_get_tasks report as failureReason.
|
/// (<c>max_turns|timeout|error</c>) 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).</summary>
|
/// "cancelled" is set explicitly at the call sites that know it (there's no CLI signal for it).</summary>
|
||||||
|
|||||||
@@ -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<WorktreeManager>.Instance);
|
||||||
|
var runner = new TaskRunner(fake, dbFactory, new HubBroadcaster(new CapturingHubContext()), wt,
|
||||||
|
new ClaudeArgsBuilder(), cfg, NullLogger<TaskRunner>.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()
|
public void Dispose()
|
||||||
{
|
{
|
||||||
foreach (var (repoDir, wtPath) in _cleanups)
|
foreach (var (repoDir, wtPath) in _cleanups)
|
||||||
|
|||||||
Reference in New Issue
Block a user