From 18ce6e13a391cb2f7a9ccea752bfbbd1fa6d8a1b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carlos=20Flor=C3=AAncio?= Date: Tue, 12 May 2026 18:54:14 +0100 Subject: [PATCH] fix(improve-kandev): lock workflow, block submit during bootstrap, hide None mode (#885) --- .../backend/internal/improvekandev/handler.go | 124 +++++++++++++++--- .../internal/improvekandev/handler_test.go | 81 ++++++++++++ .../improve-kandev-dialog-create.tsx | 11 ++ .../task-create-dialog-footer.test.ts | 39 ++++++ .../components/task-create-dialog-footer.tsx | 13 +- .../task-create-dialog-form-body.tsx | 9 ++ apps/web/components/task-create-dialog.tsx | 52 ++++++-- apps/web/components/workflow-selector-row.tsx | 7 +- apps/web/e2e/tests/improve-kandev.spec.ts | 111 +++++++++++++++- 9 files changed, 410 insertions(+), 37 deletions(-) diff --git a/apps/backend/internal/improvekandev/handler.go b/apps/backend/internal/improvekandev/handler.go index f8eb087eda..ac064f90e1 100644 --- a/apps/backend/internal/improvekandev/handler.go +++ b/apps/backend/internal/improvekandev/handler.go @@ -14,6 +14,10 @@ import ( taskservice "github.com/kandev/kandev/internal/task/service" ) +// remoteResolver resolves a local repo's origin remote into provider/owner/name. +// Injectable so tests can avoid touching real git directories. +type remoteResolver func(localPath string) (provider, owner, name string) + // Constants identifying the canonical kandev repository and the workflow // template loaded from apps/backend/config/workflows/improve-kandev.yml. const ( @@ -46,17 +50,21 @@ type Handler struct { // gh resolves the authenticated user's login and write access. Defaults // to a gh-CLI shell-out; tests can substitute a fake. gh GitHubInfo + // resolveRemote resolves a local repo path's origin remote. Defaults to + // service.ResolveGitRemoteProvider; tests can substitute a fake. + resolveRemote remoteResolver } // NewHandler constructs a Handler. version is embedded into bundle metadata. func NewHandler(taskSvc *taskservice.Service, cloner Cloner, version string, log *logger.Logger) *Handler { return &Handler{ - taskSvc: taskSvc, - cloner: cloner, - log: log, - version: version, - snapshot: func() []buffer.Entry { return buffer.Default().Snapshot() }, - gh: newDefaultGitHubInfo(), + taskSvc: taskSvc, + cloner: cloner, + log: log, + version: version, + snapshot: func() []buffer.Entry { return buffer.Default().Snapshot() }, + gh: newDefaultGitHubInfo(), + resolveRemote: taskservice.ResolveGitRemoteProvider, } } @@ -118,21 +126,7 @@ func (h *Handler) httpBootstrap(c *gin.Context) { } ctx := c.Request.Context() - localPath, err := h.cloner.EnsureCloned(ctx, repoCloneURL, repoOwner, repoName) - if err != nil { - h.log.Error("improve-kandev: clone failed", zap.Error(err)) - c.JSON(http.StatusInternalServerError, gin.H{"error": "failed to clone kandev repo"}) - return - } - - repo, err := h.taskSvc.FindOrCreateRepository(ctx, &taskservice.FindOrCreateRepositoryRequest{ - WorkspaceID: req.WorkspaceID, - Provider: repoProvider, - ProviderOwner: repoOwner, - ProviderName: repoName, - DefaultBranch: defaultBranch, - LocalPath: localPath, - }) + repo, err := h.resolveOrCloneRepo(ctx, req.WorkspaceID) if err != nil { h.log.Error("improve-kandev: repository upsert failed", zap.Error(err)) c.JSON(http.StatusInternalServerError, gin.H{"error": "failed to register kandev repository"}) @@ -182,6 +176,94 @@ func (h *Handler) httpBootstrap(c *gin.Context) { }) } +// resolveOrCloneRepo returns the workspace's kandev repository, preferring an +// existing entry — even one the user added themselves with no provider info — +// over cloning a managed copy into ~/.kandev/repos. Order: +// 1. Match by provider info (workspace + github + kdlbs + kandev). +// 2. Match by scanning workspace repos whose origin remote resolves to +// kdlbs/kandev; backfill provider info on the match. +// 3. Fall back to cloning into the managed location and registering it. +func (h *Handler) resolveOrCloneRepo(ctx context.Context, workspaceID string) (*taskmodels.Repository, error) { + if existing, err := h.taskSvc.GetRepositoryByProviderInfo(ctx, workspaceID, repoProvider, repoOwner, repoName); err != nil { + return nil, err + } else if existing != nil { + return existing, nil + } + + repos, err := h.taskSvc.ListRepositories(ctx, workspaceID) + if err != nil { + // Listing failed — log and fall through to clone path; we'd rather + // risk a duplicate than fail the bootstrap entirely. + h.log.Warn("improve-kandev: list repositories failed; falling back to clone", zap.Error(err)) + } else if match := findKandevRepoByLocalRemote(repos, h.resolveRemote); match != nil { + h.backfillKandevProviderInfo(ctx, match) + return match, nil + } + + localPath, err := h.cloner.EnsureCloned(ctx, repoCloneURL, repoOwner, repoName) + if err != nil { + return nil, err + } + return h.taskSvc.FindOrCreateRepository(ctx, &taskservice.FindOrCreateRepositoryRequest{ + WorkspaceID: workspaceID, + Provider: repoProvider, + ProviderOwner: repoOwner, + ProviderName: repoName, + DefaultBranch: defaultBranch, + LocalPath: localPath, + }) +} + +// findKandevRepoByLocalRemote returns the first repo with a local path whose +// origin remote resolves to kdlbs/kandev. Skips rows that already declare +// non-matching provider info to avoid hijacking unrelated entries. +func findKandevRepoByLocalRemote(repos []*taskmodels.Repository, resolve remoteResolver) *taskmodels.Repository { + if resolve == nil { + return nil + } + for _, r := range repos { + if r == nil || r.LocalPath == "" { + continue + } + if r.Provider != "" && (r.Provider != repoProvider || r.ProviderOwner != repoOwner || r.ProviderName != repoName) { + continue + } + p, o, n := resolve(r.LocalPath) + if p == repoProvider && o == repoOwner && n == repoName { + return r + } + } + return nil +} + +// backfillKandevProviderInfo fills missing provider/owner/name on an existing +// repo so subsequent lookups by provider info hit the fast path. Failures are +// logged but non-fatal — we still return the matched repo to the caller. +func (h *Handler) backfillKandevProviderInfo(ctx context.Context, repo *taskmodels.Repository) { + if repo.Provider == repoProvider && repo.ProviderOwner == repoOwner && repo.ProviderName == repoName { + return + } + provider, owner, name := repoProvider, repoOwner, repoName + branch := repo.DefaultBranch + if branch == "" { + branch = defaultBranch + } + if _, err := h.taskSvc.UpdateRepository(ctx, repo.ID, &taskservice.UpdateRepositoryRequest{ + Provider: &provider, + ProviderOwner: &owner, + ProviderName: &name, + DefaultBranch: &branch, + }); err != nil { + h.log.Warn("improve-kandev: backfill provider info failed", + zap.String("repository_id", repo.ID), zap.Error(err)) + return + } + repo.Provider = provider + repo.ProviderOwner = owner + repo.ProviderName = name + repo.DefaultBranch = branch +} + // emuBlockedMessage explains the EMU-restriction case to the contributor in // terms they can act on. Surfaced in the dialog when ForkStatusBlockedEMU is // returned. diff --git a/apps/backend/internal/improvekandev/handler_test.go b/apps/backend/internal/improvekandev/handler_test.go index 972dd355b0..a4b0469518 100644 --- a/apps/backend/internal/improvekandev/handler_test.go +++ b/apps/backend/internal/improvekandev/handler_test.go @@ -6,6 +6,7 @@ import ( "testing" "github.com/kandev/kandev/internal/common/logger" + taskmodels "github.com/kandev/kandev/internal/task/models" ) // fakeGitHubInfo is a configurable GitHubInfo for unit tests. Each method @@ -125,6 +126,86 @@ func TestIsEMULogin(t *testing.T) { } } +func TestFindKandevRepoByLocalRemote(t *testing.T) { + resolver := func(path string) (string, string, string) { + switch path { + case "/home/u/kandev": + return "github", "kdlbs", "kandev" + case "/home/u/fork": + return "github", "alice", "kandev" + case "/home/u/other": + return "github", "kdlbs", "other" + } + return "", "", "" + } + + cases := []struct { + name string + repos []*taskmodels.Repository + want string // matched repo ID, or "" for no match + }{ + {name: "empty list", repos: nil, want: ""}, + { + name: "skips entries without local path", + repos: []*taskmodels.Repository{ + {ID: "no-path", LocalPath: ""}, + }, + want: "", + }, + { + name: "matches remote owner/name with no provider info", + repos: []*taskmodels.Repository{ + {ID: "match", LocalPath: "/home/u/kandev"}, + }, + want: "match", + }, + { + name: "skips fork (different owner)", + repos: []*taskmodels.Repository{ + {ID: "fork", LocalPath: "/home/u/fork"}, + }, + want: "", + }, + { + name: "skips repo already wired to a different provider", + repos: []*taskmodels.Repository{ + {ID: "wired", LocalPath: "/home/u/kandev", Provider: "github", ProviderOwner: "someone", ProviderName: "kandev"}, + }, + want: "", + }, + { + name: "returns first match when multiple candidates", + repos: []*taskmodels.Repository{ + {ID: "first", LocalPath: "/home/u/kandev"}, + {ID: "second", LocalPath: "/home/u/kandev"}, + }, + want: "first", + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + got := findKandevRepoByLocalRemote(tc.repos, resolver) + if tc.want == "" { + if got != nil { + t.Errorf("expected no match, got %q", got.ID) + } + return + } + if got == nil || got.ID != tc.want { + t.Errorf("got = %v, want id = %q", got, tc.want) + } + }) + } +} + +func TestFindKandevRepoByLocalRemote_NilResolver(t *testing.T) { + repos := []*taskmodels.Repository{{ID: "x", LocalPath: "/p"}} + if got := findKandevRepoByLocalRemote(repos, nil); got != nil { + t.Errorf("nil resolver must return nil, got %v", got) + } +} + func TestIsGHNotFound(t *testing.T) { cases := []struct { name string diff --git a/apps/web/components/improve-kandev-dialog-create.tsx b/apps/web/components/improve-kandev-dialog-create.tsx index 5986cdddf6..00e4f17ce9 100644 --- a/apps/web/components/improve-kandev-dialog-create.tsx +++ b/apps/web/components/improve-kandev-dialog-create.tsx @@ -32,6 +32,16 @@ export type BootstrapState = type ImproveKind = "bug" | "feature"; +// Surfaced in the dialog's submit-button tooltip while the bootstrap probe is +// in flight, so the disabled state has an explanation the user can act on +// instead of the usual missing-field reasons. +function bootstrapBlockedReason(bootstrap: BootstrapState): string | null { + if (bootstrap.kind === "loading" || bootstrap.kind === "idle") { + return "Preparing kandev repository…"; + } + return null; +} + const PLACEHOLDERS: Record = { bug: "E.g. When I open the kanban board, the column header text overlaps the task count badge on screens narrower than 1200px...", feature: @@ -90,6 +100,7 @@ export function CreateModeView(props: CreateModeViewProps) { }} onSuccess={props.onTaskCreated} lockedFields={{ repository: true, branch: true, workflow: true }} + submitBlockedReason={bootstrapBlockedReason(bootstrap)} descriptionPlaceholder={PLACEHOLDERS[kind]} aboveDescriptionSlot={} extraFormSlot={ diff --git a/apps/web/components/task-create-dialog-footer.test.ts b/apps/web/components/task-create-dialog-footer.test.ts index 69b4395f97..4517b86df0 100644 --- a/apps/web/components/task-create-dialog-footer.test.ts +++ b/apps/web/components/task-create-dialog-footer.test.ts @@ -166,3 +166,42 @@ describe("computeDisabledReason (default)", () => { ).toBeNull(); }); }); + +describe("computeDisabledReason (submitBlockedReason)", () => { + const REASON = "Preparing kandev repository…"; + + it("returns the supplied reason for start-task even when nothing is missing", () => { + expect(computeDisabledReason(makeProps({ submitBlockedReason: REASON }), KIND_START)).toBe( + REASON, + ); + }); + + it("returns the supplied reason for default in create mode", () => { + expect(computeDisabledReason(makeProps({ submitBlockedReason: REASON }), KIND_DEFAULT)).toBe( + REASON, + ); + }); + + it("returns the supplied reason for update mode (overrides title check)", () => { + expect( + computeDisabledReason( + makeProps({ submitBlockedReason: REASON, hasTitle: false }), + KIND_UPDATE, + ), + ).toBe(REASON); + }); + + it("still suppresses the reason while a submission is in flight", () => { + expect( + computeDisabledReason( + makeProps({ submitBlockedReason: REASON, isCreatingTask: true }), + KIND_START, + ), + ).toBeNull(); + }); + + it("ignores empty/null reason and falls back to normal logic", () => { + expect(computeDisabledReason(makeProps({ submitBlockedReason: null }), KIND_START)).toBeNull(); + expect(computeDisabledReason(makeProps({ submitBlockedReason: "" }), KIND_START)).toBeNull(); + }); +}); diff --git a/apps/web/components/task-create-dialog-footer.tsx b/apps/web/components/task-create-dialog-footer.tsx index 2412dd6981..0e9a473463 100644 --- a/apps/web/components/task-create-dialog-footer.tsx +++ b/apps/web/components/task-create-dialog-footer.tsx @@ -231,6 +231,13 @@ export type TaskCreateDialogFooterProps = { onUpdateWithoutAgent: () => void; onCreateWithoutAgent: () => void; onCreateWithPlanMode?: () => void; + /** + * Externally-supplied reason that the submit buttons are disabled (e.g. an + * async bootstrap step from a feature wrapper hasn't finished yet). When set, + * every submit variant is disabled and the tooltip shows this string instead + * of the usual missing-field reason. + */ + submitBlockedReason?: string | null; }; function isMissingWorkflowCtx( @@ -290,6 +297,7 @@ export function computeDisabledReason( kind: ButtonKind, ): string | null { if (props.isCreatingTask) return null; + if (props.submitBlockedReason) return props.submitBlockedReason; if (kind === "update") return props.hasTitle ? null : REASON_TITLE; if (kind === "default" && props.isSessionMode) return sessionDefaultReason(props); const base = baseReason(props); @@ -308,9 +316,10 @@ function computeFooterState(props: TaskCreateDialogFooterProps) { const showStartTask = (props.isCreateMode && (props.hasDescription || props.isPassthroughProfile)) || Boolean(props.isEditMode && props.agentProfileId); - const altDisabled = computeBaseDisabled(props); + const blocked = Boolean(props.submitBlockedReason); + const altDisabled = computeBaseDisabled(props) || blocked; const splitDisabled = altDisabled || !props.agentProfileId; - const defaultDisabled = props.isSessionMode ? !props.agentProfileId : altDisabled; + const defaultDisabled = (props.isSessionMode ? !props.agentProfileId : altDisabled) || blocked; const disabledReason = computeDisabledReason(props, resolveButtonKind(props, showStartTask)); diff --git a/apps/web/components/task-create-dialog-form-body.tsx b/apps/web/components/task-create-dialog-form-body.tsx index 8555fa7495..81ed95fc0f 100644 --- a/apps/web/components/task-create-dialog-form-body.tsx +++ b/apps/web/components/task-create-dialog-form-body.tsx @@ -214,6 +214,13 @@ type WorkflowSectionProps = { effectiveWorkflowId: string | null; onWorkflowChange: (value: string) => void; agentProfiles: AgentProfileOption[]; + /** + * When true the picker is hidden entirely. Used by feature wrappers + * (Improve Kandev) where the workflow is enforced and the user must not be + * able to switch to a different one. The wrapper is responsible for + * surfacing the workflow elsewhere (e.g. a steps preview). + */ + workflowLocked?: boolean; }; export const WorkflowSection = memo(function WorkflowSection({ @@ -224,6 +231,7 @@ export const WorkflowSection = memo(function WorkflowSection({ effectiveWorkflowId, onWorkflowChange, agentProfiles, + workflowLocked, }: WorkflowSectionProps) { const [lastUsedWorkflowId, setLastUsedWorkflowId] = useState(null); @@ -240,6 +248,7 @@ export const WorkflowSection = memo(function WorkflowSection({ ); if (!isCreateMode || isTaskStarted) return null; + if (workflowLocked) return null; if (!effectiveWorkflowId || workflows.length > 1) { return ( diff --git a/apps/web/components/task-create-dialog.tsx b/apps/web/components/task-create-dialog.tsx index 3d8b030871..f1d6e3fed9 100644 --- a/apps/web/components/task-create-dialog.tsx +++ b/apps/web/components/task-create-dialog.tsx @@ -32,6 +32,7 @@ import { useDialogFormState, useTaskCreateDialogEffects, useDialogHandlers, + useLockedFieldSync, useSessionRepoName, useTaskCreateDialogData, computeIsTaskStarted, @@ -89,6 +90,12 @@ interface TaskCreateDialogProps { extraFormSlot?: React.ReactNode; /** Optional render slot at the bottom of the dialog footer area. */ bottomSlot?: React.ReactNode; + /** + * When set, every submit button is disabled and the tooltip surfaces this + * exact reason (e.g. an async bootstrap step from a feature wrapper hasn't + * completed yet). Takes precedence over the usual missing-field reasons. + */ + submitBlockedReason?: string | null; } type DialogFormBodyProps = { @@ -126,7 +133,7 @@ type DialogFormBodyProps = { onToggleGitHubUrl?: () => void; onGitHubUrlChange: (v: string) => void; onToggleFreshBranch: (enabled: boolean) => void; - onToggleNoRepository: () => void; + onToggleNoRepository?: () => void; onWorkspacePathChange: (value: string) => void; enhance?: { onEnhance: () => void; isLoading: boolean; isConfigured: boolean }; workflowAgentLocked: boolean; @@ -148,6 +155,8 @@ type DialogFormBodyProps = { bottomSlot?: React.ReactNode; /** Optional override for the description placeholder. */ descriptionPlaceholder?: string; + /** When true, hides the workflow picker so the enforced workflow can't be swapped. */ + workflowLocked?: boolean; }; function computeHasAllBranches(fs: DialogFormState): boolean { @@ -291,6 +300,7 @@ function DialogFormBody(props: DialogFormBodyProps) { effectiveWorkflowId={props.effectiveWorkflowId} onWorkflowChange={props.onWorkflowChange} agentProfiles={props.agentProfiles} + workflowLocked={props.workflowLocked} /> ); @@ -461,6 +471,7 @@ function useTaskCreateDialogSetup(props: TaskCreateDialogProps) { workflows, isLocalExecutor: computed.isLocalExecutor, }); + useLockedFieldSync(open, workflowId, initialValues, fs); const handlers = useDialogHandlers(fs, repositories); const submitHandlers = useSubmitHandlersWiring({ props, @@ -471,8 +482,12 @@ function useTaskCreateDialogSetup(props: TaskCreateDialogProps) { isSessionMode, isEditMode, }); + const guardedHandleSubmit = useGuardedSubmit( + submitHandlers.handleSubmit, + props.submitBlockedReason, + ); const handleKeyDown = useKeyboardShortcutHandler(SHORTCUTS.SUBMIT, (event) => { - submitHandlers.handleSubmit(event as unknown as FormEvent); + guardedHandleSubmit(event as unknown as FormEvent); }); // Fresh-branch is single-row + local executor + not URL mode. The chip row // can hold any number of repos; we hide the toggle whenever the question @@ -496,21 +511,40 @@ function useTaskCreateDialogSetup(props: TaskCreateDialogProps) { submitHandlers, handleKeyDown, freshBranchAvailable, + guardedHandleSubmit, enhance: useEnhanceForDialog(fs), handleJiraImport: useJiraImportHandler(fs), handleLinearImport: useLinearImportHandler(fs), }; } +// Buttons are disabled when submitBlockedReason is set, but the form can still +// be submitted via Enter; gate the submit path here so a wrapper's async +// bootstrap step always finishes before any task is created. +function useGuardedSubmit( + handleSubmit: (e: FormEvent) => void, + blockedReason: string | null | undefined, +) { + const blocked = Boolean(blockedReason); + return useCallback( + (e: FormEvent) => { + if (blocked) e.preventDefault(); + else handleSubmit(e); + }, + [blocked, handleSubmit], + ); +} + export function TaskCreateDialog(props: TaskCreateDialogProps) { const { open, onOpenChange, initialValues, workspaceId } = props; const setup = useTaskCreateDialogSetup(props); const { fs, isSessionMode, isEditMode, isCreateMode, isTaskStarted } = setup; const { sessionRepoName, workflows, agentProfiles, snapshots, repositories } = setup; const { computed, handlers, handleKeyDown, freshBranchAvailable } = setup; - const { handleSubmit, handleUpdateWithoutAgent, handleCreateWithoutAgent } = setup.submitHandlers; + const repoLocked = !!props.lockedFields?.repository; + const { handleUpdateWithoutAgent, handleCreateWithoutAgent } = setup.submitHandlers; const { handleCreateWithPlanMode, handleCancel } = setup.submitHandlers; - const { handleJiraImport, handleLinearImport } = setup; + const { handleJiraImport, handleLinearImport, guardedHandleSubmit } = setup; return ( -
+ diff --git a/apps/web/components/workflow-selector-row.tsx b/apps/web/components/workflow-selector-row.tsx index 194c01c7b0..d2bf99ce6b 100644 --- a/apps/web/components/workflow-selector-row.tsx +++ b/apps/web/components/workflow-selector-row.tsx @@ -117,7 +117,12 @@ export const WorkflowSelectorRow = memo(function WorkflowSelectorRow({ return ( -