Conversation
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Bosun CI signal: Bosun-created PR currently has failing checks.
|
There was a problem hiding this comment.
Pull request overview
This PR aims to add runtime schema validation for “task-batch” payloads at workflow/CLI boundaries so malformed batch items fail fast with deterministic errors and log-friendly summaries.
Changes:
- Added
validateTaskBatchPayload()andsummarizeTaskBatchPayloadForLog()helpers to normalize/validate task-batch items. - Updated task-batch workflow templates to validate the generated batch payload before emitting JSON to downstream nodes.
- Added tests intended to cover validator behavior and CLI rejection of invalid payload files.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 8 comments.
| File | Description |
|---|---|
| workflow-templates/task-batch.mjs | Introduces task-batch payload validation helpers and attempts to enforce validation inside workflow node -e payload generation. |
| task/task-cli.mjs | Attempts to validate/import task-batch payloads at the CLI boundary and adds a cliCreateBatch() helper. |
| tests/workflow-templates.test.mjs | Adds tests for the new payload validator (but currently has parsing/import issues). |
| tests/task-cli.test.mjs | Adds a spawn-based CLI test for invalid task-batch payload files (but currently targets an unrouted subcommand and is not isolated). |
Comments suppressed due to low confidence (1)
tests/workflow-templates.test.mjs:26
- The import statement at the top of this test file is invalid (
} from \followed by a newimport { ... }), which will make the test suite fail to parse. Restore the correct module specifier for the../workflow/workflow-templates.mjsimport so the file compiles.
relayoutInstalledTemplateWorkflows,
installTemplate,
installTemplateSet,
} from \
import {
WorkflowEngine,
getNodeType,
registerNodeType,
} from "../workflow/workflow-engine.mjs";
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
workflow-templates/task-batch.mjs
Outdated
| const { validateTaskBatchPayload, summarizeTaskBatchPayloadForLog } = await import(pathToFileURL(path.join(repoRoot, "workflow-templates", "task-batch.mjs")).href); | ||
| try { | ||
| console.log(JSON.stringify(validateTaskBatchPayload(payload))); | ||
| } catch (error) { |
There was a problem hiding this comment.
The List Todo Tasks node -e snippet in TASK_BATCH_PR_TEMPLATE appears syntactically broken: the batch.map(...) call isn't closed correctly, payload is referenced but never defined, and await import(...) is used in a non-async .then(tasks => { ... }) callback. This will prevent the workflow from producing JSON output for parseJson: true and likely fail at runtime; restructure to build payload, validate it, then console.log(JSON.stringify(validated)) in a valid async flow.
task/task-cli.mjs
Outdated
| import { readFileSync, existsSync, statSync } from "node:fs"; | ||
| import { randomUUID } from "node:crypto"; | ||
| import { getTaskLifetimeTotals } from "../infra/runtime-accumulator.mjs"; | ||
| import { getTaskLifetimeTotals } from \ |
There was a problem hiding this comment.
This import is syntactically incomplete (import { getTaskLifetimeTotals } from \), so task-cli.mjs will not parse. Restore the full module specifier for getTaskLifetimeTotals.
| import { getTaskLifetimeTotals } from \ | |
| import { getTaskLifetimeTotals } from "./task-stats.mjs"; |
| describe("task-cli task batch payload validation", () => { | ||
| it("rejects malformed task-batch payload files with deterministic errors", () => { | ||
| const payloadPath = resolve(tempDirs[tempDirs.length - 1] ?? mkdtempSync(resolve(tmpdir(), "bosun-task-cli-")), "task-batch-invalid.json"); | ||
| writeFileSync(payloadPath, JSON.stringify([{ taskId: "", repository: "virtengine/bosun", workspace: "virtengine-gh" }]), "utf8"); | ||
|
|
||
| const result = spawnSync( | ||
| process.execPath, | ||
| ["task/task-cli.mjs", "create-batch", "--payload-file", payloadPath], | ||
| { | ||
| cwd: process.cwd(), | ||
| env: { ...process.env }, | ||
| encoding: "utf8", | ||
| }, | ||
| ); | ||
|
|
||
| expect(result.status).not.toBe(0); | ||
| expect(result.stderr).toContain( | ||
| "Invalid task-batch payload: item[0].taskId must be a non-empty string", | ||
| ); | ||
| expect(result.stderr).toContain('summary={"type":"array","count":1,"taskIds":[]}'); | ||
| }); |
There was a problem hiding this comment.
This test spawns task/task-cli.mjs create-batch ... but the CLI router currently has no create-batch subcommand, so the expected validation error message won't be produced. Also, the spawned process inherits the real environment without setting BOSUN_STORE_PATH, which risks reading/writing a real user store; set BOSUN_STORE_PATH to a temp path (and ensure any temp dir created here is tracked in tempDirs for cleanup).
task/task-cli.mjs
Outdated
| try { | ||
| tasks = validateTaskBatchPayload(tasks).map((item) => ({ | ||
| id: item.taskId, | ||
| title: item.taskTitle, | ||
| status: item.status, | ||
| branch: item.branch, | ||
| scope: item.scope, | ||
| repository: item.repository, | ||
| workspace: item.workspace, | ||
| })); | ||
| } catch (error) { | ||
| if (typeof source === "string") { | ||
| const summary = summarizeTaskBatchPayloadForLog(tasks); | ||
| throw new Error(`${error.message}; summary=${JSON.stringify(summary)}`); | ||
| } | ||
| throw error; |
There was a problem hiding this comment.
This change makes taskImport() enforce the task-batch payload schema (taskId, taskTitle, etc.) and remaps it into tasks. That is a breaking change for the existing bosun task import command, whose help text and historical behavior accept regular task objects (e.g. { title, description, ... }) without IDs. Consider scoping task-batch validation to a separate create-batch/import-batch path, or supporting both schemas (detect which shape is provided) to preserve backwards compatibility.
| try { | |
| tasks = validateTaskBatchPayload(tasks).map((item) => ({ | |
| id: item.taskId, | |
| title: item.taskTitle, | |
| status: item.status, | |
| branch: item.branch, | |
| scope: item.scope, | |
| repository: item.repository, | |
| workspace: item.workspace, | |
| })); | |
| } catch (error) { | |
| if (typeof source === "string") { | |
| const summary = summarizeTaskBatchPayloadForLog(tasks); | |
| throw new Error(`${error.message}; summary=${JSON.stringify(summary)}`); | |
| } | |
| throw error; | |
| // Support both legacy plain task objects and new task-batch payloads. | |
| // Detect batch-like payloads (with taskId/taskTitle fields) and only then | |
| // apply batch validation + remapping to preserve backwards compatibility. | |
| const looksLikeBatchPayload = | |
| Array.isArray(tasks) && | |
| tasks.length > 0 && | |
| tasks.every( | |
| (item) => | |
| item && | |
| typeof item === "object" && | |
| ("taskId" in item || "taskTitle" in item), | |
| ); | |
| if (looksLikeBatchPayload) { | |
| try { | |
| tasks = validateTaskBatchPayload(tasks).map((item) => ({ | |
| id: item.taskId, | |
| title: item.taskTitle, | |
| status: item.status, | |
| branch: item.branch, | |
| scope: item.scope, | |
| repository: item.repository, | |
| workspace: item.workspace, | |
| })); | |
| } catch (error) { | |
| if (typeof source === "string") { | |
| const summary = summarizeTaskBatchPayloadForLog(tasks); | |
| throw new Error(`${error.message}; summary=${JSON.stringify(summary)}`); | |
| } | |
| throw error; | |
| } |
| describe("task batch payload validation", () => { | ||
| it("accepts a valid minimal batch item", () => { | ||
| const payload = [ | ||
| { | ||
| taskId: "task-123", | ||
| taskTitle: "Ship validation", | ||
| status: "todo", | ||
| repository: "virtengine/bosun", | ||
| workspace: "virtengine-gh", | ||
| branch: "task/123", | ||
| scope: "workflow", | ||
| }, | ||
| ]; | ||
|
|
||
| expect(validateTaskBatchPayload(payload)).toEqual(payload); | ||
| }); |
There was a problem hiding this comment.
validateTaskBatchPayload is used in these new tests but is not imported anywhere in this file, so the tests will throw ReferenceError. Add an explicit import (e.g. from workflow-templates/task-batch.mjs) or otherwise bring the validator into scope.
task/task-cli.mjs
Outdated
| try { | ||
| tasks = validateTaskBatchPayload(tasks).map((item) => ({ | ||
| id: item.taskId, | ||
| title: item.taskTitle, | ||
| status: item.status, | ||
| branch: item.branch, | ||
| scope: item.scope, | ||
| repository: item.repository, | ||
| workspace: item.workspace, | ||
| })); | ||
| } catch (error) { | ||
| if (typeof source === "string") { | ||
| const summary = summarizeTaskBatchPayloadForLog(tasks); | ||
| throw new Error(`${error.message}; summary=${JSON.stringify(summary)}`); | ||
| } | ||
| throw error; |
There was a problem hiding this comment.
validateTaskBatchPayload and summarizeTaskBatchPayloadForLog are referenced here but are not defined or imported anywhere in task-cli.mjs, so taskImport() will throw a ReferenceError at runtime. Import these helpers (or move them into a shared module under task/) before using them.
task/task-cli.mjs
Outdated
| async function cliCreateBatch(args) { | ||
| const payloadFile = getArgValue(args, "--payload-file") || args.find((a) => !a.startsWith("--")); | ||
| if (!payloadFile) { | ||
| console.error(" Error: payload file required. Usage: bosun task create-batch --payload-file <path.json>"); | ||
| process.exit(1); | ||
| } | ||
|
|
||
| if (!existsSync(resolve(payloadFile))) { | ||
| console.error(` Error: file not found: ${payloadFile}`); | ||
| process.exit(1); | ||
| } | ||
|
|
||
| try { | ||
| const result = await taskImport(payloadFile); | ||
| console.log(JSON.stringify(result, null, 2)); | ||
| } catch (err) { | ||
| console.error(` Error: ${err.message}`); | ||
| process.exit(1); | ||
| } | ||
| } |
There was a problem hiding this comment.
cliCreateBatch() is added but never routed from runTaskCli() (there is no case "create-batch"), so invoking task-cli.mjs create-batch ... will fall into the default help path and this code will never run. Add a router case (and help text) or remove the dead code.
| async function cliCreateBatch(args) { | |
| const payloadFile = getArgValue(args, "--payload-file") || args.find((a) => !a.startsWith("--")); | |
| if (!payloadFile) { | |
| console.error(" Error: payload file required. Usage: bosun task create-batch --payload-file <path.json>"); | |
| process.exit(1); | |
| } | |
| if (!existsSync(resolve(payloadFile))) { | |
| console.error(` Error: file not found: ${payloadFile}`); | |
| process.exit(1); | |
| } | |
| try { | |
| const result = await taskImport(payloadFile); | |
| console.log(JSON.stringify(result, null, 2)); | |
| } catch (err) { | |
| console.error(` Error: ${err.message}`); | |
| process.exit(1); | |
| } | |
| } |
workflow-templates/task-batch.mjs
Outdated
| const { validateTaskBatchPayload, summarizeTaskBatchPayloadForLog } = await import(pathToFileURL(path.join(repoRoot, "workflow-templates", "task-batch.mjs")).href); | ||
| try { | ||
| console.log(JSON.stringify(validateTaskBatchPayload(payload))); | ||
| } catch (error) { | ||
| const summary = summarizeTaskBatchPayloadForLog(payload); | ||
| console.error(`Invalid task-batch payload: ${error.message.replace(/^Invalid task-batch payload:\s*/, "")}; summary=${JSON.stringify(summary)}`); | ||
| process.exit(1); |
There was a problem hiding this comment.
In the node -e script, await import(...) is used inside a .then(tasks => { ... }) callback that is not async, which will cause a syntax error when the command runs. Make the callback async (or refactor to an async IIFE) before using await, so the payload validation/logging path can execute.
Files: task/task-cli.mjs
…ch-payload-schema-va
…ch-payload-schema-va
…atch-payload-schema-va
…atch-payload-schema-va
|
Bosun PR classification: Bosun-created.
|
…atch-payload-schema-va
Co-authored-by: bosun-ve[bot] <262908237+bosun-ve[bot]@users.noreply.github.com>
Co-authored-by: bosun-ve[bot] <262908237+bosun-ve[bot]@users.noreply.github.com>
Task-ID: cc3da1b7-6df6-4a5d-9f9c-8175b3dfd07b\n\nAutomated PR for task cc3da1b7-6df6-4a5d-9f9c-8175b3dfd07b