fix(workflow): exclude office workflows from settings export (#1184)
This commit is contained in:
@@ -76,6 +76,7 @@ func RegisterRoutes(
|
||||
})
|
||||
g.POST("/task-sessions", seedTaskSessionHandler(repo, eventBus, log))
|
||||
g.POST("/messages", seedMessageHandler(repo, eventBus, log))
|
||||
g.POST("/workflows", seedWorkflowHandler(repo, log))
|
||||
if officeRepo != nil {
|
||||
g.POST("/comments", seedCommentHandler(officeRepo, eventBus, log))
|
||||
g.POST("/agent-failures", seedAgentFailureHandler(officeRepo, eventBus, log))
|
||||
@@ -162,6 +163,47 @@ func seedTaskSessionHandler(repo *sqliterepo.Repository, eventBus bus.EventBus,
|
||||
}
|
||||
}
|
||||
|
||||
type seedWorkflowRequest struct {
|
||||
WorkspaceID string `json:"workspace_id"`
|
||||
Name string `json:"name"`
|
||||
// Style is persisted as-is (kanban / office / custom). Production has no
|
||||
// HTTP path that creates an office-style workflow — the normal create
|
||||
// endpoint always normalises to kanban — so this seed route lets the
|
||||
// Playwright suite stand up an office workflow to exercise the
|
||||
// "exclude office from settings export" behaviour (issue #1109).
|
||||
Style string `json:"style"`
|
||||
}
|
||||
|
||||
type seedWorkflowResponse struct {
|
||||
WorkflowID string `json:"workflow_id"`
|
||||
}
|
||||
|
||||
func seedWorkflowHandler(repo *sqliterepo.Repository, log *logger.Logger) gin.HandlerFunc {
|
||||
return func(c *gin.Context) {
|
||||
var req seedWorkflowRequest
|
||||
if err := c.ShouldBindJSON(&req); err != nil {
|
||||
c.JSON(http.StatusBadRequest, gin.H{"error": "invalid JSON: " + err.Error()})
|
||||
return
|
||||
}
|
||||
if req.WorkspaceID == "" || req.Name == "" {
|
||||
c.JSON(http.StatusBadRequest, gin.H{"error": "workspace_id and name are required"})
|
||||
return
|
||||
}
|
||||
workflow := &models.Workflow{
|
||||
ID: uuid.New().String(),
|
||||
WorkspaceID: req.WorkspaceID,
|
||||
Name: req.Name,
|
||||
Style: req.Style,
|
||||
}
|
||||
if err := repo.CreateWorkflow(c.Request.Context(), workflow); err != nil {
|
||||
log.Error("test harness: create workflow failed", zap.Error(err))
|
||||
c.JSON(http.StatusInternalServerError, gin.H{"error": err.Error()})
|
||||
return
|
||||
}
|
||||
c.JSON(http.StatusOK, seedWorkflowResponse{WorkflowID: workflow.ID})
|
||||
}
|
||||
}
|
||||
|
||||
func validateSeedTaskSessionRequest(req *seedTaskSessionRequest) error {
|
||||
if req.TaskID == "" {
|
||||
return errBadField("task_id is required")
|
||||
|
||||
@@ -291,6 +291,49 @@ func TestSeedMessageRejectsUnknownSession(t *testing.T) {
|
||||
}
|
||||
}
|
||||
|
||||
func TestSeedWorkflowPersistsStyle(t *testing.T) {
|
||||
repo, _ := newTestRepo(t)
|
||||
r := newRouter(t, repo, nil)
|
||||
|
||||
body := mustJSON(t, map[string]interface{}{
|
||||
"workspace_id": "ws-1",
|
||||
"name": "Office Only Workflow",
|
||||
"style": models.WorkflowStyleOffice,
|
||||
})
|
||||
w := httptest.NewRecorder()
|
||||
req := httptest.NewRequest(http.MethodPost, "/api/v1/_test/workflows", bytes.NewReader(body))
|
||||
req.Header.Set("Content-Type", "application/json")
|
||||
r.ServeHTTP(w, req)
|
||||
if w.Code != http.StatusOK {
|
||||
t.Fatalf("expected 200, got %d: %s", w.Code, w.Body.String())
|
||||
}
|
||||
|
||||
workflows, err := repo.ListWorkflows(context.Background(), "ws-1", true)
|
||||
if err != nil {
|
||||
t.Fatalf("list workflows: %v", err)
|
||||
}
|
||||
if len(workflows) != 1 {
|
||||
t.Fatalf("expected 1 workflow, got %d", len(workflows))
|
||||
}
|
||||
if got := workflows[0].Style; got != models.WorkflowStyleOffice {
|
||||
t.Fatalf("expected style %q, got %q", models.WorkflowStyleOffice, got)
|
||||
}
|
||||
}
|
||||
|
||||
func TestSeedWorkflowRejectsMissingFields(t *testing.T) {
|
||||
repo, _ := newTestRepo(t)
|
||||
r := newRouter(t, repo, nil)
|
||||
|
||||
body := mustJSON(t, map[string]interface{}{"style": "office"})
|
||||
w := httptest.NewRecorder()
|
||||
req := httptest.NewRequest(http.MethodPost, "/api/v1/_test/workflows", bytes.NewReader(body))
|
||||
req.Header.Set("Content-Type", "application/json")
|
||||
r.ServeHTTP(w, req)
|
||||
if w.Code != http.StatusBadRequest {
|
||||
t.Fatalf("expected 400 for missing workspace_id/name, got %d", w.Code)
|
||||
}
|
||||
}
|
||||
|
||||
func mustJSON(t *testing.T, v interface{}) []byte {
|
||||
t.Helper()
|
||||
b, err := json.Marshal(v)
|
||||
|
||||
@@ -236,9 +236,10 @@ func (c *Controller) ExportWorkflow(ctx context.Context, workflowID string) (*mo
|
||||
return c.svc.ExportWorkflow(ctx, workflowID)
|
||||
}
|
||||
|
||||
// ExportWorkflows exports all workflows for a workspace.
|
||||
func (c *Controller) ExportWorkflows(ctx context.Context, workspaceID string) (*models.WorkflowExport, error) {
|
||||
return c.svc.ExportWorkflows(ctx, workspaceID)
|
||||
// ExportWorkflows exports workflows for a workspace. A nil workflowIDs exports
|
||||
// every workflow; a non-nil slice restricts the export to that set of IDs.
|
||||
func (c *Controller) ExportWorkflows(ctx context.Context, workspaceID string, workflowIDs []string) (*models.WorkflowExport, error) {
|
||||
return c.svc.ExportWorkflows(ctx, workspaceID, workflowIDs)
|
||||
}
|
||||
|
||||
// ImportWorkflows imports workflows into a workspace.
|
||||
|
||||
@@ -4,6 +4,7 @@ import (
|
||||
"context"
|
||||
"io"
|
||||
"net/http"
|
||||
"strings"
|
||||
|
||||
"github.com/gin-gonic/gin"
|
||||
"go.uber.org/zap"
|
||||
@@ -240,7 +241,7 @@ func (h *Handlers) httpExportWorkflow(c *gin.Context) {
|
||||
}
|
||||
|
||||
func (h *Handlers) httpExportWorkflows(c *gin.Context) {
|
||||
resp, err := h.controller.ExportWorkflows(c.Request.Context(), c.Param("id"))
|
||||
resp, err := h.controller.ExportWorkflows(c.Request.Context(), c.Param("id"), parseExportIDs(c))
|
||||
if err != nil {
|
||||
h.logger.Error("failed to export workflows", zap.Error(err))
|
||||
c.JSON(http.StatusInternalServerError, gin.H{"error": "Failed to export workflows"})
|
||||
@@ -249,6 +250,23 @@ func (h *Handlers) httpExportWorkflows(c *gin.Context) {
|
||||
h.respondYAML(c, resp)
|
||||
}
|
||||
|
||||
// parseExportIDs reads the optional comma-separated `ids` query param. It
|
||||
// returns nil when the param is absent (export all, back-compat) and a non-nil
|
||||
// slice (possibly empty) when present, restricting the export to those IDs.
|
||||
func parseExportIDs(c *gin.Context) []string {
|
||||
raw, ok := c.GetQuery("ids")
|
||||
if !ok {
|
||||
return nil
|
||||
}
|
||||
ids := []string{}
|
||||
for _, part := range strings.Split(raw, ",") {
|
||||
if trimmed := strings.TrimSpace(part); trimmed != "" {
|
||||
ids = append(ids, trimmed)
|
||||
}
|
||||
}
|
||||
return ids
|
||||
}
|
||||
|
||||
func (h *Handlers) httpImportWorkflows(c *gin.Context) {
|
||||
const maxImportSize = 1 << 20 // 1 MB
|
||||
body, err := io.ReadAll(io.LimitReader(c.Request.Body, maxImportSize))
|
||||
|
||||
@@ -412,12 +412,26 @@ func (s *Service) ExportWorkflow(ctx context.Context, workflowID string) (*model
|
||||
return models.BuildWorkflowExport([]*taskmodels.Workflow{wf}, stepMap, s.resolveProfile), nil
|
||||
}
|
||||
|
||||
// ExportWorkflows exports all workflows for a workspace.
|
||||
func (s *Service) ExportWorkflows(ctx context.Context, workspaceID string) (*models.WorkflowExport, error) {
|
||||
workflows, err := s.workflowProvider.ListWorkflows(ctx, workspaceID, true)
|
||||
// ExportWorkflows exports workflows for a workspace. When workflowIDs is nil,
|
||||
// every (non-hidden) workflow is exported (back-compat). When workflowIDs is
|
||||
// non-nil, only workflows whose ID is in the set are exported — an empty set
|
||||
// exports nothing. Filtering is by ID membership only: the backend MUST NOT
|
||||
// branch on a workflow's style (see internal/workflow/models/phase2.go), so the
|
||||
// frontend decides which workflows (e.g. kanban-only) to include and passes
|
||||
// their IDs.
|
||||
//
|
||||
// Hidden/system workflows (e.g. improve-kandev) are only listed when the caller
|
||||
// passes explicit IDs; the back-compat "export everything" path leaves them out
|
||||
// so a bare GET of the export endpoint can't leak system flows.
|
||||
func (s *Service) ExportWorkflows(ctx context.Context, workspaceID string, workflowIDs []string) (*models.WorkflowExport, error) {
|
||||
includeHidden := workflowIDs != nil
|
||||
workflows, err := s.workflowProvider.ListWorkflows(ctx, workspaceID, includeHidden)
|
||||
if err != nil {
|
||||
return nil, fmt.Errorf("failed to list workflows: %w", err)
|
||||
}
|
||||
if workflowIDs != nil {
|
||||
workflows = filterWorkflowsByID(workflows, workflowIDs)
|
||||
}
|
||||
stepMap := make(map[string][]*models.WorkflowStep, len(workflows))
|
||||
for _, wf := range workflows {
|
||||
steps, stepsErr := s.repo.ListStepsByWorkflow(ctx, wf.ID)
|
||||
@@ -429,6 +443,22 @@ func (s *Service) ExportWorkflows(ctx context.Context, workspaceID string) (*mod
|
||||
return models.BuildWorkflowExport(workflows, stepMap, s.resolveProfile), nil
|
||||
}
|
||||
|
||||
// filterWorkflowsByID returns the subset of workflows whose ID is in ids,
|
||||
// preserving the input order.
|
||||
func filterWorkflowsByID(workflows []*taskmodels.Workflow, ids []string) []*taskmodels.Workflow {
|
||||
want := make(map[string]bool, len(ids))
|
||||
for _, id := range ids {
|
||||
want[id] = true
|
||||
}
|
||||
filtered := make([]*taskmodels.Workflow, 0, len(workflows))
|
||||
for _, wf := range workflows {
|
||||
if want[wf.ID] {
|
||||
filtered = append(filtered, wf)
|
||||
}
|
||||
}
|
||||
return filtered
|
||||
}
|
||||
|
||||
// ImportWorkflows imports workflows into a workspace. Deduplicates by name.
|
||||
func (s *Service) ImportWorkflows(ctx context.Context, workspaceID string, export *models.WorkflowExport) (*ImportResult, error) {
|
||||
if err := export.Validate(); err != nil {
|
||||
|
||||
@@ -492,7 +492,7 @@ func TestExportWorkflows(t *testing.T) {
|
||||
createStep(t, svc, &models.WorkflowStep{WorkflowID: "wf-1", Name: "Step1", Position: 0})
|
||||
createStep(t, svc, &models.WorkflowStep{WorkflowID: "wf-2", Name: "Step2", Position: 0})
|
||||
|
||||
export, err := svc.ExportWorkflows(ctx, "ws-1")
|
||||
export, err := svc.ExportWorkflows(ctx, "ws-1", nil)
|
||||
require.NoError(t, err)
|
||||
require.Len(t, export.Workflows, 2)
|
||||
|
||||
@@ -500,6 +500,56 @@ func TestExportWorkflows(t *testing.T) {
|
||||
assert.Contains(t, names, "Alpha")
|
||||
assert.Contains(t, names, "Beta")
|
||||
})
|
||||
|
||||
t.Run("filters to the requested workflow IDs", func(t *testing.T) {
|
||||
svc, _, mock := setupTestServiceWithProvider(t)
|
||||
ctx := context.Background()
|
||||
|
||||
mock.addWorkflow("wf-1", "ws-1", "Alpha")
|
||||
mock.addWorkflow("wf-2", "ws-1", "Beta")
|
||||
mock.addWorkflow("wf-3", "ws-1", "Gamma")
|
||||
|
||||
export, err := svc.ExportWorkflows(ctx, "ws-1", []string{"wf-1", "wf-3"})
|
||||
require.NoError(t, err)
|
||||
require.Len(t, export.Workflows, 2)
|
||||
|
||||
names := []string{export.Workflows[0].Name, export.Workflows[1].Name}
|
||||
assert.Contains(t, names, "Alpha")
|
||||
assert.Contains(t, names, "Gamma")
|
||||
assert.NotContains(t, names, "Beta")
|
||||
})
|
||||
|
||||
t.Run("empty (non-nil) ID list exports nothing", func(t *testing.T) {
|
||||
svc, _, mock := setupTestServiceWithProvider(t)
|
||||
ctx := context.Background()
|
||||
|
||||
mock.addWorkflow("wf-1", "ws-1", "Alpha")
|
||||
|
||||
export, err := svc.ExportWorkflows(ctx, "ws-1", []string{})
|
||||
require.NoError(t, err)
|
||||
assert.Empty(t, export.Workflows)
|
||||
})
|
||||
|
||||
t.Run("nil ID list omits hidden workflows but explicit IDs include them", func(t *testing.T) {
|
||||
svc, _, mock := setupTestServiceWithProvider(t)
|
||||
ctx := context.Background()
|
||||
|
||||
mock.addWorkflow("wf-1", "ws-1", "Alpha")
|
||||
mock.addWorkflow("hidden-1", "ws-1", "System Flow")
|
||||
mock.workflows[1].Hidden = true
|
||||
|
||||
// Back-compat path (nil) must not leak the hidden system workflow.
|
||||
all, err := svc.ExportWorkflows(ctx, "ws-1", nil)
|
||||
require.NoError(t, err)
|
||||
require.Len(t, all.Workflows, 1)
|
||||
assert.Equal(t, "Alpha", all.Workflows[0].Name)
|
||||
|
||||
// Explicitly requesting a hidden workflow's ID still exports it.
|
||||
explicit, err := svc.ExportWorkflows(ctx, "ws-1", []string{"hidden-1"})
|
||||
require.NoError(t, err)
|
||||
require.Len(t, explicit.Workflows, 1)
|
||||
assert.Equal(t, "System Flow", explicit.Workflows[0].Name)
|
||||
})
|
||||
}
|
||||
|
||||
func TestImportWorkflows(t *testing.T) {
|
||||
|
||||
@@ -4,7 +4,44 @@ vi.mock("@/lib/config", () => ({
|
||||
getBackendConfig: () => ({ apiBaseUrl: "http://backend.test" }),
|
||||
}));
|
||||
|
||||
import { deleteWorkspaceAction } from "./workspaces";
|
||||
import { deleteWorkspaceAction, exportAllWorkflowsAction } from "./workspaces";
|
||||
|
||||
describe("exportAllWorkflowsAction", () => {
|
||||
beforeEach(() => {
|
||||
vi.stubGlobal(
|
||||
"fetch",
|
||||
vi.fn(async () => new Response("workflows: []", { status: 200 })),
|
||||
);
|
||||
});
|
||||
|
||||
afterEach(() => {
|
||||
vi.unstubAllGlobals();
|
||||
vi.restoreAllMocks();
|
||||
});
|
||||
|
||||
const requestedUrl = () => {
|
||||
const fetchMock = fetch as unknown as ReturnType<typeof vi.fn>;
|
||||
expect(fetchMock).toHaveBeenCalledTimes(1);
|
||||
return new URL(String(fetchMock.mock.calls[0][0]));
|
||||
};
|
||||
|
||||
it("omits the ids param when no workflow IDs are passed (export all)", async () => {
|
||||
await exportAllWorkflowsAction("ws-1");
|
||||
expect(requestedUrl().searchParams.has("ids")).toBe(false);
|
||||
});
|
||||
|
||||
it("restricts the export to the provided workflow IDs", async () => {
|
||||
await exportAllWorkflowsAction("ws-1", ["wf-1", "wf-3"]);
|
||||
expect(requestedUrl().searchParams.get("ids")).toBe("wf-1,wf-3");
|
||||
});
|
||||
|
||||
it("sends an empty ids param so nothing is exported when the set is empty", async () => {
|
||||
await exportAllWorkflowsAction("ws-1", []);
|
||||
const url = requestedUrl();
|
||||
expect(url.searchParams.has("ids")).toBe(true);
|
||||
expect(url.searchParams.get("ids")).toBe("");
|
||||
});
|
||||
});
|
||||
|
||||
describe("deleteWorkspaceAction", () => {
|
||||
beforeEach(() => {
|
||||
|
||||
@@ -555,8 +555,22 @@ export async function exportWorkflowAction(workflowId: string): Promise<string>
|
||||
return response.text();
|
||||
}
|
||||
|
||||
export async function exportAllWorkflowsAction(workspaceId: string): Promise<string> {
|
||||
const response = await fetch(`${apiBaseUrl}/api/v1/workspaces/${workspaceId}/workflows/export`);
|
||||
export async function exportAllWorkflowsAction(
|
||||
workspaceId: string,
|
||||
workflowIds?: string[],
|
||||
): Promise<string> {
|
||||
// When workflowIds is provided, restrict the export to exactly that set
|
||||
// (the settings UI passes its kanban workflows, excluding office ones). An
|
||||
// empty list is sent intentionally as `ids=` so nothing is exported, rather
|
||||
// than omitting the param and falling back to exporting every workflow.
|
||||
// Encode the user-provided values that flow into the request URL (workspace
|
||||
// ID in the path, workflow IDs in the query) so they can't alter the request
|
||||
// target — clears CodeQL's js/request-forgery (SSRF) check on this path.
|
||||
const query =
|
||||
workflowIds !== undefined ? `?ids=${encodeURIComponent(workflowIds.join(","))}` : "";
|
||||
const response = await fetch(
|
||||
`${apiBaseUrl}/api/v1/workspaces/${encodeURIComponent(workspaceId)}/workflows/export${query}`,
|
||||
);
|
||||
if (!response.ok) throw new Error(`Export failed: ${response.statusText}`);
|
||||
return response.text();
|
||||
}
|
||||
|
||||
@@ -95,6 +95,7 @@ function buildWorkflowSteps(workflow: Workflow, definitions: StepDefinition[]):
|
||||
|
||||
function useWorkflowImportExport(
|
||||
workspace: Workspace | null,
|
||||
workflowItems: Workflow[],
|
||||
router: ReturnType<typeof useRouter>,
|
||||
toast: ReturnType<typeof useToast>["toast"],
|
||||
) {
|
||||
@@ -108,7 +109,11 @@ function useWorkflowImportExport(
|
||||
const handleExportAll = async () => {
|
||||
if (!workspace) return;
|
||||
try {
|
||||
const yamlText = await exportAllWorkflowsAction(workspace.id);
|
||||
// Export only the workflows shown in this settings view (kanban-only —
|
||||
// office workflows are filtered out upstream) and skip unsaved drafts.
|
||||
// Workflow import/export is kanban-only by design (ADR-0004).
|
||||
const exportIds = workflowItems.filter((wf) => !wf.id.startsWith("temp-")).map((wf) => wf.id);
|
||||
const yamlText = await exportAllWorkflowsAction(workspace.id, exportIds);
|
||||
setExportYaml(yamlText);
|
||||
setIsExportDialogOpen(true);
|
||||
} catch (error) {
|
||||
@@ -531,10 +536,17 @@ function useWorkspaceWorkflowsPage(
|
||||
) {
|
||||
const router = useRouter();
|
||||
const { toast } = useToast();
|
||||
// Workflow settings is kanban-only by design (ADR-0004): office-style
|
||||
// workflows are managed from the Office surface and are not importable /
|
||||
// exportable here, so we keep them out of this view entirely.
|
||||
const kanbanWorkflows = useMemo(
|
||||
() => workflows.filter((wf) => wf.style !== "office"),
|
||||
[workflows],
|
||||
);
|
||||
const { workflowItems, setWorkflowItems, setSavedWorkflowItems, isWorkflowDirty } =
|
||||
useWorkflowSettings(workflows, workspace?.id);
|
||||
useWorkflowSettings(kanbanWorkflows, workspace?.id);
|
||||
|
||||
const importExport = useWorkflowImportExport(workspace, router, toast);
|
||||
const importExport = useWorkflowImportExport(workspace, workflowItems, router, toast);
|
||||
const {
|
||||
isExportDialogOpen,
|
||||
setIsExportDialogOpen,
|
||||
|
||||
@@ -197,6 +197,25 @@ export class ApiClient {
|
||||
});
|
||||
}
|
||||
|
||||
/**
|
||||
* Seed a workflow with an explicit style (kanban / office / custom) via the
|
||||
* KANDEV_E2E_MOCK test harness. Production has no HTTP path that creates an
|
||||
* office-style workflow (the normal create endpoint always normalises to
|
||||
* kanban), so this is the only way to stand up an office workflow for the
|
||||
* "exclude office from settings export" coverage (issue #1109).
|
||||
*/
|
||||
async seedWorkflow(
|
||||
workspaceId: string,
|
||||
name: string,
|
||||
style: "kanban" | "office" | "custom",
|
||||
): Promise<{ workflow_id: string }> {
|
||||
return this.request("POST", "/api/v1/_test/workflows", {
|
||||
workspace_id: workspaceId,
|
||||
name,
|
||||
style,
|
||||
});
|
||||
}
|
||||
|
||||
async reorderWorkflows(
|
||||
workspaceId: string,
|
||||
workflowIds: string[],
|
||||
|
||||
@@ -30,6 +30,30 @@ test.describe("Workflow import/export", () => {
|
||||
await expect(dialog).not.toBeVisible();
|
||||
});
|
||||
|
||||
test("export all excludes office-style workflows", async ({ testPage, apiClient, seedData }) => {
|
||||
// Workflow settings is kanban-only by design (ADR-0004). Seed an
|
||||
// office-style workflow alongside the kanban "E2E Workflow" and confirm
|
||||
// Export All omits it (regression for issue #1109, where Export All
|
||||
// dumped every workflow — office triggers would silently drop on import).
|
||||
await apiClient.seedWorkflow(seedData.workspaceId, "Office Only Workflow", "office");
|
||||
|
||||
const page = new WorkflowSettingsPage(testPage);
|
||||
await page.goto(seedData.workspaceId);
|
||||
|
||||
// The office workflow must not even appear in the kanban settings list.
|
||||
await expect(testPage.getByText("Office Only Workflow")).toHaveCount(0);
|
||||
|
||||
await testPage.getByRole("button", { name: "Export All" }).click();
|
||||
|
||||
const dialog = testPage.getByRole("dialog");
|
||||
await expect(dialog).toBeVisible();
|
||||
const yamlContent = await dialog.locator("textarea").inputValue();
|
||||
expect(yamlContent).toContain("E2E Workflow");
|
||||
expect(yamlContent).not.toContain("Office Only Workflow");
|
||||
|
||||
await dialog.getByRole("button", { name: "Close" }).first().click();
|
||||
});
|
||||
|
||||
test("per-workflow export button shows that workflow's YAML", async ({ testPage, seedData }) => {
|
||||
const page = new WorkflowSettingsPage(testPage);
|
||||
await page.goto(seedData.workspaceId);
|
||||
|
||||
Reference in New Issue
Block a user