Conversation
|
commit: |
This comment has been minimized.
This comment has been minimized.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment has been minimized.
This comment has been minimized.
| let payload: JsonObject | undefined; | ||
| if (args.payload) { | ||
| try { | ||
| payload = JSON.parse(args.payload); |
There was a problem hiding this comment.
🟡 JSON.parse result not validated to be an object before assignment to JsonObject type
When parsing the payload argument in trigger.ts, JSON.parse(args.payload) is directly assigned to a variable typed as JsonObject. However, JSON.parse can return any valid JSON value (string, number, boolean, null, or array), not just objects.
Click to expand
Issue
At line 101, the code parses user input:
let payload: JsonObject | undefined;
if (args.payload) {
try {
payload = JSON.parse(args.payload);
} catch { ... }
}If a user provides a valid JSON string that isn't an object (e.g., "hello", 123, true, null, or [1,2,3]), the JSON.parse will succeed but the result won't be a JsonObject. This causes a type mismatch at runtime.
Impact
- User passes
--payload '123'→ parses to number123, not an object - User passes
--payload '[1,2,3]'→ parses to array, not an object - The API call at
packages/sdk/src/cli/executor/trigger.ts:40-44may fail or behave unexpectedly when receiving a non-object payload
Expected Behavior
The code should validate that the parsed JSON is actually an object before assigning it to payload.
Recommendation: Add validation after JSON.parse to ensure the result is an object:
const parsed = JSON.parse(args.payload);
if (typeof parsed !== 'object' || parsed === null || Array.isArray(parsed)) {
throw new Error('Payload must be a JSON object, not an array, primitive, or null.');
}
payload = parsed;
Was this helpful? React with 👍 or 👎 to provide feedback.
toiroakr
force-pushed
the
feat/trigger-executor-command
branch
from
4e45e41 to
72b5111
Compare
This comment has been minimized.
This comment has been minimized.
toiroakr
force-pushed
the
feat/trigger-executor-command
branch
from
72b5111 to
7277049
Compare
This comment has been minimized.
This comment has been minimized.
toiroakr
force-pushed
the
feat/trigger-executor-command
branch
from
7277049 to
7974095
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
| const jobs = await listExecutorJobs({ | ||
| executorName: args.executorName as string, | ||
| status: args.status, | ||
| limit: args.limit ? Number.parseInt(args.limit, 10) : undefined, |
There was a problem hiding this comment.
🟡 Missing validation for --limit argument allows NaN or negative values to be passed to API
The --limit argument in the executor jobs command is parsed using Number.parseInt(args.limit, 10) without validation.
Click to expand
Problem
When a user provides an invalid value like --limit abc or --limit -5, the code passes NaN or a negative number directly to the API:
limit: args.limit ? Number.parseInt(args.limit, 10) : undefined,Number.parseInt("abc", 10)returnsNaNNumber.parseInt("-5", 10)returns-5
Expected Behavior
The limit should be validated as a positive integer before being passed to the API, similar to how packages/sdk/src/cli/workspace/list.ts:14 handles it:
const limitSchema = z.coerce.number().int().positive().optional();Impact
Users will receive confusing API errors instead of clear validation messages when providing invalid limit values.
Recommendation: Add validation for the limit argument using a zod schema similar to the workspace list command: const limitSchema = z.coerce.number().int().positive().optional(); and validate before calling listExecutorJobs.
Was this helpful? React with 👍 or 👎 to provide feedback.
toiroakr
force-pushed
the
feat/trigger-executor-command
branch
2 times, most recently
from
d9117b3 to
3f0430a
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
| configPath: args.config, | ||
| dryRun: args["dry-run"], | ||
| yes: args.yes, | ||
| noSchemaCheck: args["schema-check"] === false, | ||
| }); |
There was a problem hiding this comment.
🟡 --no-schema-check flag is ignored because apply command reads a non-existent schema-check arg
The CLI defines the boolean option as "no-schema-check", but when wiring CLI args into apply(), it sets noSchemaCheck using args["schema-check"] === false.
Actual behavior:
- Passing
--no-schema-checksetsargs["no-schema-check"] === true, but this is never read. args["schema-check"]is undefined (arg not declared), soargs["schema-check"] === falseis alwaysfalse.- Result:
noSchemaCheckis alwaysfalse, so schema checks are never skipped even when the user requests it.
Expected behavior:
noSchemaCheckshould reflectargs["no-schema-check"](or the equivalent negated--schema-checkstyle arg if that existed).
Impact:
- Documented CLI behavior is broken; users cannot bypass schema checks (which can block
applyin situations where skipping is intended, e.g., CI/debugging).
Relevant code
"no-schema-check": { type: "boolean", ... }
...
noSchemaCheck: args["schema-check"] === false,(Refers to lines 334-347)
Recommendation: Change the mapping to noSchemaCheck: Boolean(args["no-schema-check"]) (or rename the option to --schema-check/--no-schema-check consistently and read the correct key). Add/adjust a unit test to ensure the flag propagates into apply().
Was this helpful? React with 👍 or 👎 to provide feedback.
toiroakr
force-pushed
the
feat/trigger-executor-command
branch
from
3f0430a to
9c5a0be
Compare
This comment has been minimized.
This comment has been minimized.
| logger.out(formatTableWithHeaders(headers, rows)); | ||
|
|
||
| // Show hint if there are webhook executors | ||
| const hasWebhook = executors.some((e) => e.triggerType === "webhook"); |
There was a problem hiding this comment.
🟡 Webhook hint detection uses inconsistent trigger type string comparison
The webhook hint in list.ts checks for triggerType === "webhook", but this string is only produced when the executor has a valid triggerConfig with case: "incomingWebhook". When triggerConfig is undefined or missing, formatTriggerTypeForList falls back to executorTriggerTypeToString() which returns "incomingWebhook" instead of "webhook".
Click to expand
Expected vs Actual
Expected: The hint "To see webhook URLs, run: tailor-sdk executor webhook list" should appear whenever there are executors with INCOMING_WEBHOOK trigger type.
Actual: The hint will not appear if an incoming webhook executor has missing/undefined triggerConfig, because:
formatTriggerTypeForList(transform.ts:131-147) returns"webhook"only whenconfig.case === "incomingWebhook"- Otherwise it falls back to
executorTriggerTypeToString()(status.ts:163-164) which returns"incomingWebhook" - The check
e.triggerType === "webhook"inlist.ts:74would then fail
Impact
This is a minor UX issue - users may not see the helpful hint about viewing webhook URLs in edge cases where the executor has an INCOMING_WEBHOOK trigger type but missing config.
Recommendation: Change the comparison to also check for "incomingWebhook": const hasWebhook = executors.some((e) => e.triggerType === "webhook" || e.triggerType === "incomingWebhook" || e.triggerType.startsWith("webhook"));
Was this helpful? React with 👍 or 👎 to provide feedback.
- Rename --watch to --wait for consistency with workflow commands - Add -W alias for --wait, -i alias for --interval - Add --logs/-l option to display function/workflow execution logs - Remove type cast for jobId (proto now includes the field) - Replace printData with logger.out - Replace table import with formatKeyValueTable Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Add PENDING_RESUME status to terminal state checks in watchExecutorJob and waitForCompletion functions to prevent infinite loops when a workflow enters the PENDING_RESUME state. - Use isWorkflowExecutionTerminalStatus helper in workflow/executions.ts - Add PENDING_RESUME handling with appropriate warning message in executor/jobs.ts - Remove unused exports to fix knip errors Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Add executor command documentation (trigger, jobs) - Update CLI reference to include executor commands section - Use waitForExecution for workflow targets to show real-time progress (status changes and running job names, same as workflow start --wait) - Fix log/result indentation in output for better readability - Change spinner text from "Polling..." to "Waiting for workflow to complete..." Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Add --data (-d) and --header (-H) options (curl-compatible)
- Headers can be specified multiple times: -H "Key: Value"
- Internally combines into { body: {...}, headers: {...} } structure
- Update documentation with new options and examples
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Add trigger type validation for executor trigger command: - EVENT trigger type cannot be triggered manually - SCHEDULE trigger type does not accept --data/--header options - Add --limit option for executor jobs command (default: 50, max: 1000) - Remove fetchAll to avoid fetching all jobs at once - Update documentation with trigger type restrictions Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Align executor CLI commands with the main branch's politty migration. Updates index.ts, jobs.ts, and trigger.ts to use Zod schemas and politty's defineCommand API. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
toiroakr
force-pushed
the
feat/trigger-executor-command
branch
from
9c5a0be to
58482d5
Compare
This comment has been minimized.
This comment has been minimized.
Use humanizeRelativeTime from format.ts instead of duplicating formatDistanceToNowStrict logic in logger.ts. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Code Metrics Report (packages/sdk)
Details | | main (502050f) | #494 (4c7a273) | +/- |
|--------------------|----------------|----------------|-------|
- | Coverage | 49.9% | 47.8% | -2.1% |
| Files | 198 | 204 | +6 |
| Lines | 6563 | 6852 | +289 |
+ | Covered | 3278 | 3281 | +3 |
- | Code to Test Ratio | 1:0.3 | 1:0.3 | -0.1 |
| Code | 36225 | 37144 | +919 |
| Test | 13606 | 13606 | 0 |Code coverage of files in pull request scope (16.0% → 8.1%)
SDK Configure Bundle Size
Runtime Performance
Type Performance (instantiations)
Reported by octocov |
2 participants
This pull request introduces a new set of CLI commands for managing executors and their jobs, along with supporting code and documentation. It also adds a new test executor and updates integration tests to reflect these changes. The most important changes are grouped below.
New Executor CLI Commands:
executorCLI command withtriggerandjobssubcommands for managing executors and their jobs, including options for payloads, waiting for job completion, and viewing logs (packages/sdk/src/cli/executor/index.ts,packages/sdk/src/cli/executor/trigger.ts,packages/sdk/src/cli/executor/status.ts,packages/sdk/src/cli/executor/transform.ts,packages/sdk/src/cli/executor/jobs.ts) [1] [2] [3] [4].executorcommand in the main CLI entrypoint (packages/sdk/src/cli/index.ts) [1] [2].Documentation Updates:
packages/sdk/docs/cli/executor.md).packages/sdk/docs/cli-reference.md).SDK Exports:
packages/sdk/src/cli/lib.ts).Testing and Example Executors:
testWebhook.tsthat demonstrates an incoming webhook trigger (example/executors/testWebhook.ts).example/e2e/executor.test.ts,example/tests/apply_command.test.ts) [1] [2].Workflow Execution Improvements:
PENDING_RESUME(packages/sdk/src/cli/workflow/executions.ts) [1] [2].