feat(task): add create, ls, info, update, and rm commands - #1315
feat(task): add create, ls, info, update, and rm commands#1315Ayush7614 wants to merge 4 commits into
Conversation
Expand the task namespace beyond run so users can manage saved Actor tasks from the CLI instead of only Console, with shared resolve helpers and e2e coverage.
l2ysho
left a comment
There was a problem hiding this comment.
Hi @Ayush7614 thx for contribution. I seems good enough, I left some comments about topic I found important.
| let resolved; | ||
| try { | ||
| resolved = await resolveTaskId(apifyClient, taskId); | ||
| } catch (err) { | ||
| error({ message: (err as Error).message, stdout: true }); | ||
| return; | ||
| } |
There was a problem hiding this comment.
important: catching resolveTaskId's throw turns exit 1 into exit 0.
task run (src/commands/task/run.ts:43) lets the same error propagate, so ApifyCommand._run catches it at src/lib/command-framework/apify-command.ts:386-391, prints Error: <msg> and sets process.exitCode ||= 1. Here we catch the identical error, print the identical message, and return — nothing sets the exit code (grep process.exitCode src/commands/task/ → zero hits).
So apify task info missing exits 0 while apify task run missing exits 1, for the same failure in the same namespace. And because of stdout: true, apify task info missing --json | jq gets non-JSON on stdout plus a success status.
The catch buys nothing the framework doesn't already do — dropping it is a net line reduction. (CommandExitCodes.NotFound = 250 at src/lib/consts.ts:94 if you'd rather be explicit.)
Same pattern in task/rm.ts:52-58 and task/update.ts:85-91.
| let resolved; | ||
| try { | ||
| resolved = await resolveTaskId(client, taskId); | ||
| } catch (err) { | ||
| error({ message: (err as Error).message, stdout: true }); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Same exit-code downgrade as task/info.ts:46-52 — see that comment. Deleting this try/catch makes update behave like task run.
| let resolved; | ||
| try { | ||
| resolved = await resolveTaskId(apifyClient, taskId); | ||
| } catch (err) { | ||
| error({ message: (err as Error).message, stdout: true }); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Same exit-code downgrade as task/info.ts:46-52 — see that comment.
| } catch (err) { | ||
| const casted = err as ApifyApiError; | ||
| error({ | ||
| message: `Failed to delete Task "${resolved.userFriendlyId}".\n ${casted.message || casted}`, | ||
| stdout: true, | ||
| }); | ||
| } |
There was a problem hiding this comment.
important: 403 or 5xx on delete prints an error message and exits 0, so a script that does apify task rm x --yes && echo gone reports success on a failed deletion.
Either process.exitCode ||= 1 here, or drop the catch and let the framework format the ApifyApiError.
| function parseJsonInput(raw: string, sourceLabel: string): Dictionary | Dictionary[] { | ||
| try { | ||
| return JSON.parse(raw) as Dictionary | Dictionary[]; | ||
| } catch { | ||
| throw new Error(`Failed to parse ${sourceLabel} as JSON.`); | ||
| } | ||
| } |
There was a problem hiding this comment.
medium — this reimplements getInputOverride and is byte-identical to task/update.ts:13-19.
src/lib/commands/resolve-input.ts already exports getInputOverride(cwd, input, inputFile, opts?), used by task/run.ts, actors/call.ts and actors/start.ts, and it has unit tests (test/local/lib/resolve-input.test.ts). The hand-rolled version loses:
- bare piped stdin —
echo '{}' | apify task create x --actor ypicks up nothing.getInputOverridehandles the no-flags-given case atresolve-input.ts:60-77; there's no equivalent here. - the empty-stdin diagnostic for
--input -— the framework substitutes piped content beforerun()sees the flag (apify-command.ts:572-583), so a value that still starts with-means nothing was piped.resolve-input.ts:95says exactly that; this code reachesJSON.parse('-')and reportsFailed to parse --input as JSON.instead. - cwd-relative path resolution
- the "don't pass a
.jsonpath to--input" guard - treating a non-path
--input-filevalue as inline JSON process.exitCode = CommandExitCodes.InvalidInputon bad input- the underlying parser message —
Failed to parse --input as JSON.discardserr.message, so the user never learns where the JSON broke
Also: neither file imports node:process, and readFileSync(inputFile) resolves against the real cwd — CLAUDE.md requires the explicit import process from 'node:process' for the test cwd mocks to work.
Swapping in getInputOverride is ~5 lines per file and deletes 14 (plus the readFileSync import).
One caveat worth checking: getInputOverride rejects top-level JSON arrays, while apify-client types task input as Dictionary | Dictionary[]. I found no evidence the platform actually accepts array task input (the API reference describes an object), so I believe this is theoretical rather than a real regression.
Edited: the first bullet originally claimed the flags omit stdin: StdinMode.Stringified so --input - is "taken literally". That was wrong — Stringified is the default for Flags.string, and --input - works. Replaced with the two cases that genuinely differ.
| } | ||
|
|
||
| success({ | ||
| message: `Task ${chalk.yellow(resolved.userFriendlyId)} (${chalk.gray(updated.id)}) was updated.`, |
There was a problem hiding this comment.
| message: `Task ${chalk.yellow(resolved.userFriendlyId)} (${chalk.gray(updated.id)}) was updated.`, | |
| message: `Task ${chalk.yellow(updated.name)} (${chalk.gray(updated.id)}) was updated.`, |
| for (const task of rawTaskList.items) { | ||
| table.pushRow({ | ||
| 'Task ID': task.id, | ||
| Name: task.name ? `${user.username ?? task.username}/${task.name}` : chalk.italic('Unnamed'), |
There was a problem hiding this comment.
| Name: task.name ? `${user.username ?? task.username}/${task.name}` : chalk.italic('Unnamed'), | |
| Name: `${task.username}/${task.name}`, |
| import { runCli } from '../../__helpers__/run-cli.js'; | ||
| import { createTestActor, removeTestActor, type TestActor } from '../../__helpers__/test-actor.js'; | ||
|
|
||
| describe('[e2e][api] task namespace', () => { |
There was a problem hiding this comment.
Could you add tests for the parts that aren't covered?
The one new test is [e2e][api], which only runs under test:e2e — not
test:local or test:api, the two suites CI gates PRs on. So none of this
code is currently exercised on a PR run.
Most of it doesn't need a token. src/lib/commands/ siblings unit-test their
helpers with an as unknown as ApifyClient stub (run-on-cloud.test.ts:5-12),
and getLocalUserInfo() is satisfiable with writeAuthFile({ username }) +
__APIFY_INTERNAL_TEST_AUTH_PATH__ (credentials.test.ts:41-51).
test/local/lib/resolve-task.test.ts — stub client, no token:
- resolves
username/name - resolves a bare ID
- resolves a bare name
- lowercases the bare name (
resolve-task.ts:50) — the e2e task name is
already lowercase, so nothing catches a regression here today - throws when the task doesn't exist
-
tryToGetTaskreturnsnullinstead of throwing
test/local/commands/task/*.test.ts:
-
--inputwith malformed JSON -
--input-filewith malformed JSON -
--inputand--input-filetogether (theexclusivepairing — nothing
in the repo coversexclusiveat all) -
task updatewith no flags → the "provide at least one of" error -
task createwith a name that already exists - the human-readable output of
create/update/ls/info—
right now all four are asserted only through--json
Sibling suites pair a failure case with nearly every success case if you want
a shape to follow: builds/lifecycle.test.ts:73,82,116,166,211.
| const pushResult = await runCli('apify', ['push'], { | ||
| cwd: actor.dir, | ||
| env: authEnv, | ||
| }); | ||
|
|
||
| if (pushResult.exitCode !== 0) { | ||
| throw new Error(`Push failed:\n${pushResult.stderr}\n${pushResult.stdout}`); | ||
| } | ||
| }, 300_000); |
There was a problem hiding this comment.
nit — this apify push is what forces the 300_000 timeout, and nothing in the suite needs it: a task only needs an actId, the suite never runs the task, and create.ts never touches builds.
client.actors().create({ name }) — or just using apify/hello-world as --actor and dropping the test actor entirely — would cut most of the runtime.
| it('deletes a task', async () => { | ||
| const result = await runCli('apify', ['task', 'rm', taskName, '--yes'], { env: authEnv }); | ||
| expect(result.exitCode, `stderr: ${result.stderr}`).toBe(0); | ||
| expect(result.stdout + result.stderr).toMatch(/deleted/i); |
There was a problem hiding this comment.
| expect(result.stdout + result.stderr).toMatch(/deleted/i); | |
| expect(result.stdout).toContain('was deleted'); |
Propagate resolveTaskId failures, set non-zero exit on delete/create errors, reuse getInputOverride with -i/-f shortcuts, drop redundant create preflight, add local coverage, and simplify the e2e suite.
|
@l2ysho thanks for the thorough review — all of the feedback should be addressed in the latest commits:
|
Summary
apify tasknamespace beyondtask runwith full manage commands:create,ls,info,update, andrm.resolveTaskId/tryToGetTaskhelper (also used bytask run) so tasks resolve consistently by ID,username/name, or bare name.This fills a real product gap: tasks are first-class on the platform, but the CLI previously only supported running them. No open issue was claimed for this work.
Test plan
pnpm run lintpnpm run formatpnpm run buildpnpm run test:local(375+ tests passed)apify taskhelp lists the new subcommandspnpm run test:e2e/ API token suite fortest/e2e/commands/task/lifecycle.test.ts(requiresTEST_USER_TOKEN)