From 2b9d5f1a5db670602b3dda6e95dff53efdaa7c1a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carlos=20Flor=C3=AAncio?= Date: Sat, 12 Sep 2026 16:24:56 +0100 Subject: [PATCH] fix: scope PR auto-fix outcome reporting (#3602) --- .../github/store_ci_outcome_scope_test.go | 213 ++++++++++++++++++ .../mcp/handlers/task_pr_automation.go | 7 +- .../task_pr_automation_outcome_scope_test.go | 33 +++ .../mcp/handlers/task_pr_automation_test.go | 2 +- .../server/pr_auto_fix_outcome_scope_test.go | 44 ++++ apps/backend/internal/mcp/server/server.go | 6 +- .../ci_automation_outcome_scope_test.go | 35 +++ .../event_handlers_github_ci_automation.go | 2 + docs/plans/pr-auto-fix-outcome-scope/plan.md | 168 ++++++++++++++ .../task-01-scope-reporting-guidance.md | 93 ++++++++ .../task-02-explain-unmatched-reports.md | 104 +++++++++ .../specs/ui/requirements/ci-pr-automation.md | 17 +- .../ui/system-design/ci-pr-automation-02.md | 41 +++- 13 files changed, 757 insertions(+), 8 deletions(-) create mode 100644 apps/backend/internal/github/store_ci_outcome_scope_test.go create mode 100644 apps/backend/internal/mcp/handlers/task_pr_automation_outcome_scope_test.go create mode 100644 apps/backend/internal/mcp/server/pr_auto_fix_outcome_scope_test.go create mode 100644 apps/backend/internal/orchestrator/ci_automation_outcome_scope_test.go create mode 100644 docs/plans/pr-auto-fix-outcome-scope/plan.md create mode 100644 docs/plans/pr-auto-fix-outcome-scope/task-01-scope-reporting-guidance.md create mode 100644 docs/plans/pr-auto-fix-outcome-scope/task-02-explain-unmatched-reports.md diff --git a/apps/backend/internal/github/store_ci_outcome_scope_test.go b/apps/backend/internal/github/store_ci_outcome_scope_test.go new file mode 100644 index 0000000000..1573cb3b7a --- /dev/null +++ b/apps/backend/internal/github/store_ci_outcome_scope_test.go @@ -0,0 +1,213 @@ +package github + +import ( + "context" + "errors" + "reflect" + "testing" + "time" +) + +type ciOutcomeScopeSnapshot struct { + legacyOptionRows int + legacyOptions []TaskCIOptions + prOptions []*TaskPRAutomationOptions + states []*TaskCIPRAutomationState +} + +func snapshotCIAutoFixScope(t *testing.T, store *Store, taskID string) ciOutcomeScopeSnapshot { + t.Helper() + var legacyOptionRows int + if err := store.db.GetContext(context.Background(), &legacyOptionRows, + "SELECT COUNT(*) FROM github_task_ci_options WHERE task_id = ?", taskID); err != nil { + t.Fatalf("count legacy options: %v", err) + } + var legacyOptions []TaskCIOptions + if err := store.ro.SelectContext(context.Background(), &legacyOptions, + store.ro.Rebind("SELECT * FROM github_task_ci_options WHERE task_id = ?"), taskID); err != nil { + t.Fatalf("list legacy options: %v", err) + } + prOptions, err := store.ListTaskPRAutomationOptions(context.Background(), taskID) + if err != nil { + t.Fatalf("list PR options: %v", err) + } + states, err := store.ListTaskCIPRStates(context.Background(), taskID) + if err != nil { + t.Fatalf("list PR states: %v", err) + } + return ciOutcomeScopeSnapshot{ + legacyOptionRows: legacyOptionRows, + legacyOptions: legacyOptions, + prOptions: prOptions, + states: states, + } +} + +func requireCIAutoFixScopeUnchanged(t *testing.T, before, after ciOutcomeScopeSnapshot) { + t.Helper() + if !reflect.DeepEqual(before, after) { + t.Fatalf("auto-fix state changed after rejected report:\nbefore: %+v\nafter: %+v", before, after) + } +} + +// @covers AC-UI-CI-PR-AUTOMATION-001.10 +// @covers AC-UI-CI-PR-AUTOMATION-001.17 +func TestStorePRAutoFixOutcomeOrdinaryTurnHasNoSideEffects(t *testing.T) { + ctx := context.Background() + report := func(store *Store, taskID, sessionID, turnID string) error { + return store.ReportTaskCIAutoFixOutcome(ctx, TaskCIAutoFixOutcomeReport{ + TaskID: taskID, SessionID: sessionID, TurnID: turnID, + Outcome: TaskCIAutoFixOutcomeBlocked, Summary: "ordinary PR work", + }) + } + + t.Run("absent settings", func(t *testing.T) { + store := newTestStore(t) + before := snapshotCIAutoFixScope(t, store, "task-absent") + if err := report(store, "task-absent", "session-current", "turn-current"); !errors.Is(err, ErrTaskCIAutoFixAttemptNotFound) { + t.Fatalf("report error = %v, want %v", err, ErrTaskCIAutoFixAttemptNotFound) + } + after := snapshotCIAutoFixScope(t, store, "task-absent") + requireCIAutoFixScopeUnchanged(t, before, after) + }) + + t.Run("explicitly disabled settings", func(t *testing.T) { + store := newTestStore(t) + disabled := false + if _, err := store.UpdateTaskPRAutomationOptions(ctx, "task-disabled", "repo-a", 101, + TaskPRAutomationOptionsPatch{AutoFixEnabled: &disabled}, false); err != nil { + t.Fatalf("seed disabled options: %v", err) + } + before := snapshotCIAutoFixScope(t, store, "task-disabled") + if err := report(store, "task-disabled", "session-current", "turn-current"); !errors.Is(err, ErrTaskCIAutoFixAttemptNotFound) { + t.Fatalf("report error = %v, want %v", err, ErrTaskCIAutoFixAttemptNotFound) + } + after := snapshotCIAutoFixScope(t, store, "task-disabled") + requireCIAutoFixScopeUnchanged(t, before, after) + }) + + t.Run("enabled without attempt", func(t *testing.T) { + store := newTestStore(t) + enabled := true + if _, err := store.UpdateTaskPRAutomationOptions(ctx, "task-enabled", "repo-a", 101, + TaskPRAutomationOptionsPatch{AutoFixEnabled: &enabled}, false); err != nil { + t.Fatalf("seed enabled options: %v", err) + } + before := snapshotCIAutoFixScope(t, store, "task-enabled") + if err := report(store, "task-enabled", "session-current", "turn-current"); !errors.Is(err, ErrTaskCIAutoFixAttemptNotFound) { + t.Fatalf("report error = %v, want %v", err, ErrTaskCIAutoFixAttemptNotFound) + } + after := snapshotCIAutoFixScope(t, store, "task-enabled") + requireCIAutoFixScopeUnchanged(t, before, after) + }) + + t.Run("foreign bound attempt across differently configured PRs", func(t *testing.T) { + store := newTestStore(t) + enabled := true + disabled := false + for _, target := range []struct { + repositoryID string + prNumber int + value *bool + }{ + {repositoryID: "repo-a", prNumber: 101, value: &enabled}, + {repositoryID: "repo-b", prNumber: 202, value: &disabled}, + } { + if _, err := store.UpdateTaskPRAutomationOptions(ctx, "task-foreign", target.repositoryID, target.prNumber, + TaskPRAutomationOptionsPatch{AutoFixEnabled: target.value}, false); err != nil { + t.Fatalf("seed %s#%d options: %v", target.repositoryID, target.prNumber, err) + } + } + if err := store.RecordTaskCIFixAttempt(ctx, TaskCIFixAttempt{ + TaskID: "task-foreign", RepositoryID: "repo-a", PRNumber: 101, + Signature: "foreign-feedback", CheckpointJSON: `{}`, + SessionID: "session-foreign", TurnID: "turn-foreign", + ProviderGeneration: "head-foreign", State: TaskCIAutoFixAttemptRunning, + IncrementRound: true, EnqueuedAt: time.Date(2024, 1, 1, 12, 0, 0, 0, time.UTC), + }); err != nil { + t.Fatalf("seed foreign attempt: %v", err) + } + before := snapshotCIAutoFixScope(t, store, "task-foreign") + if err := report(store, "task-foreign", "session-current", "turn-current"); !errors.Is(err, ErrTaskCIAutoFixAttemptNotFound) { + t.Fatalf("report error = %v, want %v", err, ErrTaskCIAutoFixAttemptNotFound) + } + after := snapshotCIAutoFixScope(t, store, "task-foreign") + requireCIAutoFixScopeUnchanged(t, before, after) + }) + + t.Run("already reported attempt", func(t *testing.T) { + store := newTestStore(t) + attempt := TaskCIFixAttempt{ + TaskID: "task-reported", RepositoryID: "repo-a", PRNumber: 101, + Signature: "reported-feedback", CheckpointJSON: `{}`, + SessionID: "session-current", TurnID: "turn-current", + ProviderGeneration: "head-reported", State: TaskCIAutoFixAttemptRunning, + IncrementRound: true, + } + if err := store.RecordTaskCIFixAttempt(ctx, attempt); err != nil { + t.Fatalf("seed reported attempt: %v", err) + } + if err := store.ReportTaskCIAutoFixOutcome(ctx, TaskCIAutoFixOutcomeReport{ + TaskID: attempt.TaskID, SessionID: attempt.SessionID, TurnID: attempt.TurnID, + Outcome: TaskCIAutoFixOutcomeNonActionable, Summary: "first report", + }); err != nil { + t.Fatalf("record first outcome: %v", err) + } + before := snapshotCIAutoFixScope(t, store, attempt.TaskID) + if err := store.ReportTaskCIAutoFixOutcome(ctx, TaskCIAutoFixOutcomeReport{ + TaskID: attempt.TaskID, SessionID: attempt.SessionID, TurnID: attempt.TurnID, + Outcome: TaskCIAutoFixOutcomeActionTaken, Summary: "duplicate report", + }); !errors.Is(err, ErrTaskCIAutoFixAttemptNotFound) { + t.Fatalf("duplicate report error = %v, want %v", err, ErrTaskCIAutoFixAttemptNotFound) + } + after := snapshotCIAutoFixScope(t, store, attempt.TaskID) + requireCIAutoFixScopeUnchanged(t, before, after) + }) +} + +// @covers AC-UI-CI-PR-AUTOMATION-001.10 +func TestStorePRAutoFixOutcomeAcceptsAllValidOutcomes(t *testing.T) { + for _, test := range []struct { + name string + outcome TaskCIAutoFixOutcome + wantState TaskCIAutoFixAttemptState + wantLastError bool + }{ + {name: "action taken", outcome: TaskCIAutoFixOutcomeActionTaken, wantState: TaskCIAutoFixAttemptAwaitingProviderProgress}, + {name: "non actionable", outcome: TaskCIAutoFixOutcomeNonActionable, wantState: TaskCIAutoFixAttemptAcknowledged}, + {name: "blocked", outcome: TaskCIAutoFixOutcomeBlocked, wantState: TaskCIAutoFixAttemptAcknowledged, wantLastError: true}, + } { + t.Run(test.name, func(t *testing.T) { + store := newTestStore(t) + ctx := context.Background() + attempt := TaskCIFixAttempt{ + TaskID: "task-valid-" + test.name, RepositoryID: "repo-valid", PRNumber: 303, + Signature: "valid-feedback", CheckpointJSON: `{}`, + SessionID: "session-valid", TurnID: "turn-valid", + ProviderGeneration: "head-valid", State: TaskCIAutoFixAttemptRunning, + } + if err := store.RecordTaskCIFixAttempt(ctx, attempt); err != nil { + t.Fatalf("seed attempt: %v", err) + } + if err := store.ReportTaskCIAutoFixOutcome(ctx, TaskCIAutoFixOutcomeReport{ + TaskID: attempt.TaskID, SessionID: attempt.SessionID, TurnID: attempt.TurnID, + Outcome: test.outcome, Summary: "valid outcome", + }); err != nil { + t.Fatalf("record %s outcome: %v", test.outcome, err) + } + state, err := store.GetTaskCIPRState(ctx, attempt.TaskID, attempt.RepositoryID, attempt.PRNumber) + if err != nil { + t.Fatalf("get state: %v", err) + } + if state == nil { + t.Fatal("attempt state is missing") + } + if state.AutoFixAttemptState != test.wantState || state.AutoFixAttemptOutcome != test.outcome { + t.Fatalf("state = %+v, want state %q and outcome %q", state, test.wantState, test.outcome) + } + if (state.LastError != nil) != test.wantLastError { + t.Fatalf("last error presence = %t, want %t", state.LastError != nil, test.wantLastError) + } + }) + } +} diff --git a/apps/backend/internal/mcp/handlers/task_pr_automation.go b/apps/backend/internal/mcp/handlers/task_pr_automation.go index 5ab2ca6e33..1ee51a6d7d 100644 --- a/apps/backend/internal/mcp/handlers/task_pr_automation.go +++ b/apps/backend/internal/mcp/handlers/task_pr_automation.go @@ -29,6 +29,8 @@ type TaskPRAutoFixOutcomeService interface { ReportTaskPRAutoFixOutcome(ctx context.Context, taskID, sessionID, outcome, summary string) error } +const taskPRAutoFixOutcomeUnmatchedMessage = "No matching unresolved GitHub PR auto-fix attempt exists for this turn. Finish ordinary work without retrying this report or enabling auto-fix." + func (h *Handlers) SetTaskPRAutomationService(automation TaskPRAutomationService) { h.taskPRAutomation = automation } @@ -153,8 +155,9 @@ func (h *Handlers) handleReportTaskPRAutoFixOutcome(ctx context.Context, msg *ws ctx, req.TaskID, req.SessionID, req.Outcome, req.Summary, ); err != nil { switch { - case errors.Is(err, github.ErrTaskCIAutoFixAttemptNotFound), - errors.Is(err, github.ErrTaskCIAutoFixOutcomeInvalid): + case errors.Is(err, github.ErrTaskCIAutoFixAttemptNotFound): + return ws.NewError(msg.ID, msg.Action, ws.ErrorCodeValidation, taskPRAutoFixOutcomeUnmatchedMessage, nil) + case errors.Is(err, github.ErrTaskCIAutoFixOutcomeInvalid): return ws.NewError(msg.ID, msg.Action, ws.ErrorCodeValidation, err.Error(), nil) default: return ws.NewError(msg.ID, msg.Action, ws.ErrorCodeInternalError, "Failed to record PR auto-fix outcome: "+err.Error(), nil) diff --git a/apps/backend/internal/mcp/handlers/task_pr_automation_outcome_scope_test.go b/apps/backend/internal/mcp/handlers/task_pr_automation_outcome_scope_test.go new file mode 100644 index 0000000000..620e86bf3f --- /dev/null +++ b/apps/backend/internal/mcp/handlers/task_pr_automation_outcome_scope_test.go @@ -0,0 +1,33 @@ +package handlers + +import ( + "strings" + "testing" + + "github.com/kandev/kandev/internal/github" + ws "github.com/kandev/kandev/pkg/websocket" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestReportPRAutoFixOutcomeExplainsUnmatchedTurn(t *testing.T) { + automation := &recordingTaskPRAutomationService{outcomeErr: github.ErrTaskCIAutoFixAttemptNotFound} + h := &Handlers{taskPRAutoFixOutcome: automation, logger: testLogger(t).WithFields()} + + response, err := h.handleReportTaskPRAutoFixOutcome( + taskPRAutoFixOutcomeContext(), + makeWSMessage(t, ws.ActionMCPReportPRAutoFixOutcome, map[string]any{ + "outcome": "blocked", + "summary": "provider is unavailable", + }), + ) + + require.NoError(t, err) + require.Equal(t, ws.MessageTypeError, response.Type) + var payload ws.ErrorPayload + require.NoError(t, response.ParsePayload(&payload)) + assert.Equal(t, ws.ErrorCodeValidation, payload.Code) + assert.Contains(t, payload.Message, "No matching unresolved GitHub PR auto-fix attempt exists for this turn") + assert.Contains(t, payload.Message, "Finish ordinary work without retrying this report or enabling auto-fix") + assert.NotContains(t, strings.ToLower(payload.Message), "disabled") +} diff --git a/apps/backend/internal/mcp/handlers/task_pr_automation_test.go b/apps/backend/internal/mcp/handlers/task_pr_automation_test.go index 30c138c283..de8805f311 100644 --- a/apps/backend/internal/mcp/handlers/task_pr_automation_test.go +++ b/apps/backend/internal/mcp/handlers/task_pr_automation_test.go @@ -203,7 +203,7 @@ func TestHandleReportTaskPRAutoFixOutcomeRejectsInvalidAndStaleReports(t *testin })) require.NoError(t, err) assert.Equal(t, ws.MessageTypeError, response.Type) - assert.Contains(t, string(response.Payload), "not found") + assert.Contains(t, string(response.Payload), "Finish ordinary work without retrying this report or enabling auto-fix") } func TestHandleReportTaskPRAutoFixOutcomeRejectsSpoofedIdentity(t *testing.T) { diff --git a/apps/backend/internal/mcp/server/pr_auto_fix_outcome_scope_test.go b/apps/backend/internal/mcp/server/pr_auto_fix_outcome_scope_test.go new file mode 100644 index 0000000000..addac8a8e2 --- /dev/null +++ b/apps/backend/internal/mcp/server/pr_auto_fix_outcome_scope_test.go @@ -0,0 +1,44 @@ +package mcp + +import ( + "errors" + "testing" + + "github.com/mark3labs/mcp-go/mcp" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestReportPRAutoFixOutcomeToolDescriptionScopesCurrentTurn(t *testing.T) { + s := newTaskModeServer(t, &testBackend{}, "task-current") + + tool, ok := s.mcpServer.ListTools()["report_pr_auto_fix_outcome_kandev"] + require.True(t, ok, "report outcome tool must be registered") + description := tool.Tool.Description + + assert.Contains(t, description, "current Kandev-dispatched auto-fix turn") + assert.Contains(t, description, "server-owned outcome protocol") + assert.Contains(t, description, "manual PR fixup") + assert.Contains(t, description, "sibling review") + assert.Contains(t, description, "earlier auto-fix turn") + assert.Contains(t, description, "Tool availability or enabled automation settings alone do not establish an obligation to report") + assert.Contains(t, description, "exactly once") +} + +func TestReportPRAutoFixOutcomeServerSurfacesBackendError(t *testing.T) { + // Keep this transport fixture synchronized with handlers.taskPRAutoFixOutcomeUnmatchedMessage. + const explanation = "No matching unresolved GitHub PR auto-fix attempt exists for this turn. Finish ordinary work without retrying this report or enabling auto-fix." + backend := &testBackend{err: errors.New(explanation)} + s := newTaskModeServer(t, backend, "task-current") + + result := callTool(t, s, "report_pr_auto_fix_outcome_kandev", map[string]interface{}{ + "outcome": "blocked", + "summary": "provider is unavailable", + }) + + require.True(t, result.IsError) + require.Len(t, result.Content, 1) + content, ok := result.Content[0].(mcp.TextContent) + require.True(t, ok) + assert.Contains(t, content.Text, explanation) +} diff --git a/apps/backend/internal/mcp/server/server.go b/apps/backend/internal/mcp/server/server.go index 7e99c688e2..9179ac145b 100644 --- a/apps/backend/internal/mcp/server/server.go +++ b/apps/backend/internal/mcp/server/server.go @@ -1371,10 +1371,14 @@ func (s *Server) registerPRAutomationTools() { s.mcpServer.AddTool( mcp.NewTool("report_pr_auto_fix_outcome_kandev", mcp.WithDescription( - "Report the one explicit outcome for the current GitHub PR auto-fix turn. "+ + "Report the one explicit outcome for the current Kandev-dispatched auto-fix turn. "+ + "Use this tool only when this turn received the server-owned outcome protocol. "+ + "Do not use it for manual PR fixup, sibling review messages, or instructions carried over from an earlier auto-fix turn. "+ + "Tool availability or enabled automation settings alone do not establish an obligation to report. "+ "Use action_taken when a concrete provider-visible change was made, "+ "non_actionable when the feedback identifies no change this task can make, "+ "or blocked when an external condition prevents the needed change. "+ + "Report exactly once for the dispatched turn. "+ "The task, session, turn, and PR are bound by Kandev and are not tool arguments.", ), mcp.WithString("outcome", mcp.Required(), mcp.Enum("action_taken", "non_actionable", "blocked"), mcp.Description("The disposition of this auto-fix turn.")), diff --git a/apps/backend/internal/orchestrator/ci_automation_outcome_scope_test.go b/apps/backend/internal/orchestrator/ci_automation_outcome_scope_test.go new file mode 100644 index 0000000000..60abf64111 --- /dev/null +++ b/apps/backend/internal/orchestrator/ci_automation_outcome_scope_test.go @@ -0,0 +1,35 @@ +package orchestrator + +import ( + "strings" + "testing" + + "github.com/kandev/kandev/internal/sysprompt" +) + +func TestCIAutomationOutcomeProtocolScopesCurrentTurn(t *testing.T) { + for _, passthrough := range []bool{false, true} { + t.Run(map[bool]string{false: "structured", true: "passthrough"}[passthrough], func(t *testing.T) { + prompt := ciAutomationAppendOutcomeProtocol("repair the PR", passthrough) + visible := prompt + if !passthrough { + visible = sysprompt.StripSystemContent(prompt) + } + + assertions := []string{ + "current Kandev-dispatched auto-fix turn", + "expires when this turn ends", + "manual PR fixup", + "sibling review", + "historical auto-fix instructions", + "Tool availability or enabled automation settings alone do not establish this obligation", + "report_pr_auto_fix_outcome_kandev exactly once", + } + for _, want := range assertions { + if !strings.Contains(prompt, want) { + t.Errorf("protocol does not contain %q: %s", want, visible) + } + } + }) + } +} diff --git a/apps/backend/internal/orchestrator/event_handlers_github_ci_automation.go b/apps/backend/internal/orchestrator/event_handlers_github_ci_automation.go index 849169a268..3f1f695496 100644 --- a/apps/backend/internal/orchestrator/event_handlers_github_ci_automation.go +++ b/apps/backend/internal/orchestrator/event_handlers_github_ci_automation.go @@ -1203,10 +1203,12 @@ func ciAutomationRenderPromptTemplate(base, snapshot string) string { } const ciAutomationOutcomeProtocol = `Kandev PR auto-fix outcome protocol: +This instruction applies only to the current Kandev-dispatched auto-fix turn that received this protocol. It expires when this turn ends. Before this turn ends, call report_pr_auto_fix_outcome_kandev exactly once with one of these outcomes: - action_taken: you made or requested a concrete provider-visible change and want Kandev to wait for CI or PR progress. - non_actionable: the current feedback does not identify a change this task can make. - blocked: a concrete change is needed, but an external condition prevents it. Include a short reason. +Do not report an outcome for manual PR fixup, sibling review messages, or historical auto-fix instructions. Tool availability or enabled automation settings alone do not establish this obligation. Do not claim action_taken from a plan or an attempted command alone. If the tool is unavailable, continue the repair work and explain that the outcome could not be recorded.` func ciAutomationAppendOutcomeProtocol(prompt string, passthrough bool) string { diff --git a/docs/plans/pr-auto-fix-outcome-scope/plan.md b/docs/plans/pr-auto-fix-outcome-scope/plan.md new file mode 100644 index 0000000000..a2a5219eb2 --- /dev/null +++ b/docs/plans/pr-auto-fix-outcome-scope/plan.md @@ -0,0 +1,168 @@ +--- +created: 2026-09-11 +status: complete +requirements: + - REQ-UI-CI-PR-AUTOMATION-001 +system_design: + - ../../specs/ui/system-design/ci-pr-automation-02.md +legacy_specs: [] +--- + +# Fix plan: PR auto-fix outcome scope + +## Overview + +Clarify when an agent must report a PR auto-fix outcome. +First correct the tool and dispatch guidance. Then explain rejected calls and +prove that ordinary turns cannot change auto-fix state. +The implementation applies the approved guidance and rejection-scope package +without changing automation eligibility or retry behavior. + +## Diagnosis and evidence + +Affected task: `93a74cfc-6135-452e-85d1-ccd6d7af78cd`, linked PR #3590. +Session: `2edf3b96-66b5-4409-9238-36c97f4d3a63`. +Turn: `71c201a0-4115-4ca3-bb61-bdc60dd48d6c`. + +- The original user requested implementation, PR creation, a 15-minute wait, + and manual PR fixup. This request did not enable automatic PR repair. +- The affected turn started at `2026-09-11T09:28:21Z` from a sibling review + message, `9f209c9c-d7d3-54b6-bee4-f3b6a7501a2d`. +- At `2026-09-11T10:12:12Z`, message + `5865c632-ea68-4140-a507-6a4b6391180d` called + `report_pr_auto_fix_outcome_kandev` with `action_taken`. + Its summary described manual review fixes and head `46ce139`. +- The tool returned `VALIDATION_ERROR` with + `CI auto-fix attempt not found or no longer matches`. +- Read-only inspection of `/root/.kandev/data/kandev.db` found zero automation + option rows and zero attempt-state rows for this task. PR #3590 remained linked. + The legacy task settings row was absent too. `GetTaskPRAutomationOptions` + returns disabled defaults for a missing row. +- The recorded turn metadata had no auto-fix binding. The complete primary + conversation contained the original user prompt and sibling review prompt, + with no Kandev auto-fix dispatch message. + +Current database state alone cannot prove every historical switch value. +The transcript, turn record, rejected call, and disabled defaults agree with the +user's report. No evidence indicates a scheduler dispatch with auto-fix disabled. +No settings, sessions, GitHub objects, or live records changed during diagnosis. + +## Root cause + +The agent used an automatic-attempt reporting tool to summarize manual fixup. +`registerPRAutomationTools` exposes the tool to GitHub-capable task sessions, +independent of automation settings or an active attempt. Its description names +an auto-fix turn but does not explicitly exclude ordinary PR work. +The existing dispatch protocol also lacks an explicit expiry across later turns. +That latter omission is a preventive concern, not a proven trigger in this incident. + +The conditional store write correctly rejected the call. This repair addresses +agent guidance and error recovery, not an observed authorization bypass. +The exact internal reason for the model's choice is not independently provable. + +## Requirement reconciliation + +`AC-UI-CI-PR-AUTOMATION-001.10` already requires exact turn binding. +The implementation preserved that boundary in this incident. +Criteria `.16` and `.17` clarify guidance and rejection behavior that were missing. +The existing UI-owned automation pair remains authoritative during migration. +The integration index owns adjacent provider signals, not a second copy of this +attempt contract. No new ADR is needed because the existing outcome ADR already +separates ordinary turns from automatic attempts. + +## Scope + +### In scope + +- Explicit current-turn eligibility in tool and immutable dispatch guidance. +- A specific explanation for an unmatched outcome report. +- Regression coverage for valid and invalid report contexts. + +### Out of scope + +- Dynamic catalog removal, a new eligibility endpoint, or session restarts. +- Automatic settings changes or conversion of manual work into an attempt. +- Changes to retry counts, provider progress, queue delivery, or GitLab behavior. +- UI layout, translated browser copy, and modification of repository skills. +- Repairing or changing the affected task or PR. + +## Technical approach + +Keep the existing provider-scoped catalog and backend identity checks. +Update the reporting tool description in `internal/mcp/server/server.go`. +Update `ciAutomationOutcomeProtocol` in +`internal/orchestrator/event_handlers_github_ci_automation.go`. +Map the missing-attempt sentinel separately in +`internal/mcp/handlers/task_pr_automation.go`. +Retain the sentinel, error code, schema, and all failure semantics. + +Proposed rejection meaning: no matching unresolved auto-fix attempt exists for +this turn. Use this tool only for a current Kandev auto-fix dispatch. +For ordinary PR work, finish normally without retrying this report or enabling +auto-fix. Do not claim that the switch is off from this error alone. + +## Tests + +| Criteria | Planned evidence | +| --- | --- | +| `.16` | `TestReportPRAutoFixOutcomeToolDescriptionScopesCurrentTurn` in new `internal/mcp/server/pr_auto_fix_outcome_scope_test.go` | +| `.16` | `TestCIAutomationOutcomeProtocolScopesCurrentTurn` in new `internal/orchestrator/ci_automation_outcome_scope_test.go` | +| `.17` | `TestReportPRAutoFixOutcomeExplainsUnmatchedTurn` in new `internal/mcp/handlers/task_pr_automation_outcome_scope_test.go` | +| `.10`, `.17` | `TestStorePRAutoFixOutcomeOrdinaryTurnHasNoSideEffects` in new `internal/github/store_ci_outcome_scope_test.go` | +| `.10` | `TestStorePRAutoFixOutcomeAcceptsAllValidOutcomes` in new `internal/github/store_ci_outcome_scope_test.go` | + +The first three tests must fail before the planned correction. +The store matrix protects the already-working rejection boundary. +Exercise a real temporary SQLite store for state assertions. +Cover absent and disabled settings, enabled-without-attempt, another turn's +attempt, an already-reported attempt, and all three valid outcomes. +Include two PRs with different settings and a foreign bound attempt. + +## End-to-end evidence + +The observable surface is the agent tool protocol. Use the existing in-process +MCP client pattern (`callTool`) to verify the returned error and caller binding. +Store tests prove persistent effects. No browser rendering changes require +Playwright, and deterministic tests do not claim to prove every model choice. + +## Work orders + +- [x] [Task 01: Scope agent reporting guidance](task-01-scope-reporting-guidance.md) +- [x] [Task 02: Explain unmatched outcome reports](task-02-explain-unmatched-reports.md) + +Execute sequentially. The existing +[outcome retry package](../pr-auto-fix-outcome-retries/plan.md) is implemented. +This package adds clarification without reopening or rewriting its results. + +## Verification results + +Implementation: complete. Both work orders are done. + +- `rtk python3 scripts/lint-spec-files.test.py`: passed, 36 tests. +- `rtk python3 scripts/lint-spec-files.py --all`: passed. +- `rtk git diff --check`: passed. +- `rtk go test ./internal/mcp/handlers -run 'Test.*Report.*PRAutoFix' -count=1`: passed, 7 tests. +- `rtk go test ./internal/mcp/server -run 'TestReportPRAutoFixOutcome|TestTaskPRAutomationTools' -count=1`: passed, 5 tests. +- `rtk go test ./internal/github -run 'TestStorePRAutoFixOutcomeOrdinaryTurnHasNoSideEffects|TestStoreTaskCIAutoFixAttempt' -count=1`: passed, 9 tests. +- `rtk go test ./internal/orchestrator -run 'TestCIAutomationOutcomeProtocol' -count=1`: passed, 4 tests. +- `rtk make -C apps/backend build`: passed; the environment reported only the existing missing codesign/rcodesign warning for Darwin artifacts. +- `rtk make -C apps/backend test`: the changed packages passed, but the full suite exited 2 in unrelated existing areas: process-tree probing, config/home discovery, launcher service configuration, and an Office SQLite migration. +- `rtk gofmt -l` on all changed Go files: passed. +- Work-order references and existing code symbols checked against the workspace. + +## Risks + +- Clear guidance reduces misuse but cannot guarantee a model never calls the tool. + The backend guard remains mandatory. +- A static catalog must remain usable when a later auto-fix turn reuses a session. +- Historical instructions can remain in model context. The protocol must state + its current-turn scope without weakening reporting for valid dispatches. +- A missing attempt can mean stale or completed work, not only disabled settings. + +## Documentation impact + +Implementation changes are limited to agent-facing backend guidance, rejection +classification, regression coverage, and the delivery record. Public guidance +remains accurate because automatic retries and controls do not change. The +implementation retains the distinction between manual fixup and +Kandev-dispatched auto-fix in agent-facing text. diff --git a/docs/plans/pr-auto-fix-outcome-scope/task-01-scope-reporting-guidance.md b/docs/plans/pr-auto-fix-outcome-scope/task-01-scope-reporting-guidance.md new file mode 100644 index 0000000000..eb93dfc9e6 --- /dev/null +++ b/docs/plans/pr-auto-fix-outcome-scope/task-01-scope-reporting-guidance.md @@ -0,0 +1,93 @@ +--- +id: "01-scope-reporting-guidance" +title: "Scope agent reporting guidance" +status: done +wave: 1 +depends_on: [] +plan: "plan.md" +requirements: + - REQ-UI-CI-PR-AUTOMATION-001 +acceptance_criteria: + - AC-UI-CI-PR-AUTOMATION-001.16 +system_design: + - ../../specs/ui/system-design/ci-pr-automation-02.md +--- + +# Task 01: Scope agent reporting guidance + +## Summary + +Explain the current-turn reporting requirement at discovery and dispatch. +Keep the GitHub task catalog stable across manual and automatic turns. + +## In scope + +- Tool description exclusions for manual fixup, sibling review, and historical instructions. +- Explicit expiry of the immutable protocol at the end of its dispatched turn. +- Structured, passthrough, and customized-prompt coverage. + +## Out of scope + +- Catalog filtering, new tool arguments, setting changes, and retry logic. + +## Acceptance + +- Discovery explains that availability or enabled settings do not establish a reporting obligation. +- A valid current auto-fix dispatch still requires exactly one outcome. +- Prior-turn protocol text does not instruct later manual turns to report. + +## TDD sequence + +Add the two guidance tests named in the plan. Run them and record the expected failures. +Then change the two instruction surfaces and rerun the tests. +Retain existing provider membership and protocol visibility coverage. + +## Verification + +Run from the repository root: + +```bash +(cd apps/backend && rtk go test ./internal/mcp/server -run 'TestReportPRAutoFixOutcome|TestTaskPRAutomationTools|TestServerModeTask_ProviderMembership' -count=1) +(cd apps/backend && rtk go test ./internal/orchestrator -run 'TestCIAutomationOutcomeProtocol' -count=1) +rtk python3 scripts/lint-spec-files.py --all +rtk git diff --check +``` + +## Files likely touched + +- `apps/backend/internal/mcp/server/server.go` +- `apps/backend/internal/mcp/server/pr_auto_fix_outcome_scope_test.go` (new) +- `apps/backend/internal/orchestrator/event_handlers_github_ci_automation.go` +- `apps/backend/internal/orchestrator/ci_automation_outcome_scope_test.go` (new) + +## Dependencies + +None. + +## Risks + +Overly broad exclusions can suppress legitimate automatic reports. +Assert the positive reporting instruction as well as its scope. + +## Parallelism + +`sequential` + +## Inputs + +- Requirement `.16` and design section `Outcome reporting scope`. +- `TestCIAutomationOutcomeProtocolVisibility` and `newTaskModeServer`. +- [Incident evidence and test matrix](plan.md). + +## Results + +Done. The GitHub outcome tool description now scopes reporting to the current +Kandev-dispatched auto-fix turn and excludes manual fixup, sibling review, and +historical instructions. The immutable structured and passthrough dispatch +protocols state the same scope and expiry. Existing provider membership and +positive outcome visibility remain covered. + +- `rtk go test ./internal/mcp/server -run 'TestReportPRAutoFixOutcome|TestTaskPRAutomationTools|TestServerModeTask_ProviderMembership' -count=1`: passed, 10 tests. +- `rtk go test ./internal/orchestrator -run 'TestCIAutomationOutcomeProtocol' -count=1`: passed, 4 tests. +- `rtk python3 scripts/lint-spec-files.py --all`: passed. +- `rtk git diff --check`: passed. diff --git a/docs/plans/pr-auto-fix-outcome-scope/task-02-explain-unmatched-reports.md b/docs/plans/pr-auto-fix-outcome-scope/task-02-explain-unmatched-reports.md new file mode 100644 index 0000000000..06f934522c --- /dev/null +++ b/docs/plans/pr-auto-fix-outcome-scope/task-02-explain-unmatched-reports.md @@ -0,0 +1,104 @@ +--- +id: "02-explain-unmatched-reports" +title: "Explain unmatched outcome reports" +status: done +wave: 2 +depends_on: + - "01-scope-reporting-guidance" +plan: "plan.md" +requirements: + - REQ-UI-CI-PR-AUTOMATION-001 +acceptance_criteria: + - AC-UI-CI-PR-AUTOMATION-001.10 + - AC-UI-CI-PR-AUTOMATION-001.17 +system_design: + - ../../specs/ui/system-design/ci-pr-automation-02.md +--- + +# Task 02: Explain unmatched outcome reports + +## Summary + +Return a scope explanation when the backend cannot match an unresolved attempt. +Prove that rejected reports leave automation settings and attempt state unchanged. + +## In scope + +- A separate error mapping for `ErrTaskCIAutoFixAttemptNotFound`. +- Handler and MCP-client coverage for rejection guidance and `VALIDATION_ERROR`. +- Real-store regression coverage for the context matrix in the plan. + +## Out of scope + +- Successful no-op responses, synthetic attempts, schema changes, and automatic retries. +- Changing the conditional store write or caller identity contract. + +## Acceptance + +- The error explains scope without asserting that auto-fix is disabled. +- Invalid reports create no attempt, consume no round, and change no settings. +- Valid bound reports retain all three outcomes and reject duplicate or foreign-turn reports. + +## TDD sequence + +First add `TestReportPRAutoFixOutcomeExplainsUnmatchedTurn` and record its expected failure. +Add the state matrix through the real temporary store. +Change only the missing-attempt error mapping, then run all commands below. +Extend the MCP client test to prove the explanation survives the tool transport. + +## Verification + +Run from the repository root: + +```bash +(cd apps/backend && rtk go test ./internal/mcp/handlers -run 'Test.*Report.*PRAutoFix' -count=1) +(cd apps/backend && rtk go test ./internal/mcp/server -run 'TestReportPRAutoFixOutcome|TestTaskPRAutomationTools' -count=1) +(cd apps/backend && rtk go test ./internal/github -run 'TestStorePRAutoFixOutcome|TestStoreTaskCIAutoFixAttempt' -count=1) +rtk python3 scripts/lint-spec-files.test.py +rtk python3 scripts/lint-spec-files.py --all +rtk git diff --check +``` + +## Files likely touched + +- `apps/backend/internal/mcp/handlers/task_pr_automation.go` +- `apps/backend/internal/mcp/handlers/task_pr_automation_outcome_scope_test.go` (new) +- `apps/backend/internal/mcp/server/pr_auto_fix_outcome_scope_test.go` +- `apps/backend/internal/github/store_ci_outcome_scope_test.go` (new) + +## Dependencies + +Task 01 owns the shared new MCP test file and the guidance terminology. + +## Risks + +The sentinel covers missing, stale, and completed attempts. +The explanation must not recommend changing settings to repair a reporting error. +State assertions must compare rows before and after rejected calls, including absent rows. + +## Parallelism + +`sequential` + +## Inputs + +- Criteria `.10` and `.17`, and design section `Outcome reporting scope`. +- `handleReportTaskPRAutoFixOutcome`, `ReportTaskCIAutoFixOutcome`, and existing handler tests. +- `TestStoreTaskCIAutoFixAttemptLifecycleUsesExactIdentity`. +- [Incident evidence and test matrix](plan.md). + +## Results + +Done. Unmatched auto-fix outcome reports now keep `VALIDATION_ERROR` while +returning a scope explanation that covers missing, stale, and completed turns. +The explanation directs ordinary work to continue without retrying the report +or changing automation settings. The MCP transport and temporary-SQLite tests +prove that the existing task/session/turn guard and all three valid outcomes +remain intact. + +- `rtk go test ./internal/mcp/handlers -run 'Test.*Report.*PRAutoFix' -count=1`: passed, 7 tests. +- `rtk go test ./internal/mcp/server -run 'TestReportPRAutoFixOutcome|TestTaskPRAutomationTools' -count=1`: passed, 5 tests. +- `rtk go test ./internal/github -run 'TestStorePRAutoFixOutcomeOrdinaryTurnHasNoSideEffects|TestStoreTaskCIAutoFixAttempt' -count=1`: passed, 9 tests. +- `rtk python3 scripts/lint-spec-files.test.py`: passed, 36 tests. +- `rtk python3 scripts/lint-spec-files.py --all`: passed. +- `rtk git diff --check`: passed. diff --git a/docs/specs/ui/requirements/ci-pr-automation.md b/docs/specs/ui/requirements/ci-pr-automation.md index 29484c6374..4d59db9b25 100644 --- a/docs/specs/ui/requirements/ci-pr-automation.md +++ b/docs/specs/ui/requirements/ci-pr-automation.md @@ -2,7 +2,7 @@ status: active system: ui created: 2026-06-18 -updated: 2026-09-06 +updated: 2026-09-11 owners: - tbd --- @@ -64,6 +64,19 @@ Consequences section). - **AC-UI-CI-PR-AUTOMATION-001.14:** Every retry shall consume the existing per-PR 10-round budget. Kandev shall never create an 11th accepted round, and disabling then re-enabling auto-fix shall reset the retry state with the existing checkpoint and round state. - **AC-UI-CI-PR-AUTOMATION-001.15:** Existing installations whose built-in `ci-auto-fix` prompt still matches a shipped, untouched legacy revision shall receive the current default content on startup. Kandev shall preserve edited prompt rows, and the server-owned outcome instructions shall apply even when a task or global prompt is customized. -## System design +### Outcome scope clarification + +The following criteria clarify the existing turn-bound outcome contract. +The [scope repair plan](../../../plans/pr-auto-fix-outcome-scope/plan.md) records the implementation and regression coverage. + +- **AC-UI-CI-PR-AUTOMATION-001.16:** Agent guidance shall restrict outcome reporting to the current Kandev-dispatched auto-fix turn. Manual PR fixup, sibling review messages, and historical auto-fix instructions shall not establish that obligation. Tool availability and enabled automation settings alone shall not establish that obligation. +- **AC-UI-CI-PR-AUTOMATION-001.17:** When no matching unresolved attempt exists, Kandev shall reject the report with a scope explanation. The explanation shall direct the agent to finish ordinary work without retrying the report or enabling automation. The rejected report shall not create an attempt, consume a round, or change automation settings. + +## Implementation plans + +- [Outcome retries](../../../plans/pr-auto-fix-outcome-retries/plan.md): existing implementation. +- [Outcome scope repair](../../../plans/pr-auto-fix-outcome-scope/plan.md): implementation record. + +## System-design references The migrated technical source is split into [part 1](../system-design/ci-pr-automation-01.md), [part 2](../system-design/ci-pr-automation-02.md), [part 3](../system-design/ci-pr-automation-03.md). diff --git a/docs/specs/ui/system-design/ci-pr-automation-02.md b/docs/specs/ui/system-design/ci-pr-automation-02.md index 4e6acbe735..65157a3a68 100644 --- a/docs/specs/ui/system-design/ci-pr-automation-02.md +++ b/docs/specs/ui/system-design/ci-pr-automation-02.md @@ -4,7 +4,7 @@ system: ui requirements: - REQ-UI-CI-PR-AUTOMATION-001 created: 2026-06-18 -updated: 2026-09-06 +updated: 2026-09-11 owners: - tbd --- @@ -144,7 +144,44 @@ Auto-merge cycle for one task/PR: 4. If the PR is ready and the readiness signature is new, Kandev calls the existing PR merge operation using the backend default merge-method selection. 5. Kandev records the merge attempt and refreshes PR state after a successful merge when practical. -## Permissions +## Outcome reporting scope + +This clarification maps to `AC-UI-CI-PR-AUTOMATION-001.10`, +`AC-UI-CI-PR-AUTOMATION-001.16`, and `AC-UI-CI-PR-AUTOMATION-001.17`. +Its delivery record is the [scope repair plan](../../../plans/pr-auto-fix-outcome-scope/plan.md). + +`registerPRAutomationTools` exposes the reporting tool through the GitHub task +catalog. Catalog membership describes capability, not an active auto-fix attempt. +The catalog remains stable across ordinary and automatic turns in one session. + +The tool description must require a current Kandev auto-fix prompt with the +server-owned outcome protocol. It must explicitly exclude manual PR fixup and +sibling review work. A setting, a quoted marker, or an earlier turn's protocol +does not prove that the current turn owns an attempt. + +`ciAutomationOutcomeProtocol` must state that its reporting instruction applies +only to the turn receiving that dispatch. Structured and passthrough sessions +retain their existing transport behavior. Custom prompts cannot remove this scope. + +`Service.ReportTaskPRAutoFixOutcome` continues to resolve the active turn. +`Store.ReportTaskCIAutoFixOutcome` remains the authoritative conditional write. +The exact task, session, turn, running state, and empty outcome must match. +No caller-supplied identity, new attempt, or success fallback is introduced. + +`handleReportTaskPRAutoFixOutcome` must map +`ErrTaskCIAutoFixAttemptNotFound` to a specific scope explanation while retaining +`VALIDATION_ERROR`. The explanation covers missing, stale, and completed attempts +without claiming that auto-fix is disabled. It directs the agent to finish ordinary +work without another report or a settings change. Other validation and internal +errors retain their existing classification. + +Regression coverage must exercise missing settings, explicit disabled settings, +an enabled PR without an attempt, and a valid bound attempt. A mixed-PR case must +prove that an attempt on another session or turn cannot authorize this call. +No UI composition, persistence schema, retry policy, or provider catalog changes +are required. + +## Access permissions - Any user who can view and interact with the task chat can read and update the task CI automation options for that task. - Any user who can edit prompts in Settings > Prompts can edit the default `ci-auto-fix` prompt.