Repository navigation
[Hackathon] fix: read-by-id commands forward an empty id, leaking an internal dry-run error instead of a VALIDATION_ERROR #368
Description
Verified against (commit SHA or released version)
commit 988c0355bb05631e9c6c1785e443937ff2a88c0e — @testsprite/testsprite-cli 0.14.0
Discord Username / User ID
smilecat__
What does this improvement do?
Every read-by-id command (project get, test get, test code get, test result, test steps, test failure, schedule get, schedule delete) interpolates the positional <id> straight into the request path with no client-side guard. When the argument is an empty string — e.g. testsprite project get "$PROJECT_ID" with an unset shell variable — the CLI sends a malformed request (…/projects/, …/tests//code) instead of failing fast.
The sibling write commands (test delete, project delete, test update, test code put, schedule update, testlist update) already validate the id via the requireNonEmpty(...) helper; the read commands were missed. This is the same accepted class as the "raw/internal error instead of a typed error" bug (#87).
Under --dry-run the missing guard surfaces as a developer-only error:
$ testsprite project get "" --dry-run
[dry-run] sample response — not from the server
Error: No dry-run sample registered for GET https://api.testsprite.com/api/cli/v1/projects/
Add a sample to `src/lib/dry-run/samples.ts` and re-run.
requestId: req_dry-run
# exit 1
The message tells the user to edit the CLI's own source. It should instead be the clean envelope the sibling commands already emit:
$ testsprite test delete "" --dry-run
Error: Invalid request.
<test-id> is required
# exit 5 (VALIDATION_ERROR)
Details / implementation notes
Affected (verified against 988c035, with --dry-run): project get, test get, test code get, test result, test steps, test failure, schedule get, schedule delete — each forwards the empty id and exits 1 with the internal sample error.
Correctly validated today: project delete, test delete, test update, test code put, schedule update, testlist update — exit 5, <…-id> is required.
Root cause: src/commands/test.ts already has
function requireNonEmpty(flagName: string, value: string): void {
if (typeof value !== 'string' || value.trim().length === 0) {
throw localValidationError(flagName, 'is required');
}
}called by the write commands (e.g. runDelete at test.ts:2466). The read functions runGet (test.ts:5261), runCodeGet (test.ts:5300), runFailureGet (test.ts:6846), the test result / test steps paths, plus project.ts runGet (project.ts:203) and the schedule readers never call it.
Proposed fix:
- Hoist
requireNonEmptytosrc/lib/(e.g.src/lib/args.ts) sotest.ts,project.ts, andschedule.tsshare one copy. - Call it at the top of every read-by-id command, using the same
<project-id>/<test-id>/<schedule-id>label convention the delete commands already use.
Tests: unit tests per command asserting an empty ("") and whitespace-only (" ") id throws VALIDATION_ERROR (exit 5) and never reaches client.get / client.delete (spy asserted).
Related, same root cause (may fold into the same change or a follow-up): test run "", test rerun "", testlist get "", and testlist delete "" currently return a canned dry-run success for an empty id (the sample matcher matches the empty-suffix path), i.e. they "succeed" against a non-existent id.
Scope: a single change (add the missing guard consistently) — one PR, not split.
Confirmations
- I have searched existing issues and this is not a duplicate.
- I have provided my Discord identity above for reward coordination.
Activity
zeshi-du commentedon Oct 20, 2026
Thanks @Smile-Catpy — confirmed on v0.15.0 and accepted.
project get,test get,test code get,test failure,schedule getand the other read-by-id commands pass an empty id straight into the request path, while the write commands guard it.Scope (one PR):
requireNonEmptyout oftest.ts) and call it at the top of each read-by-id command, before the client is created, with the same<…-id>labels.VALIDATION_ERROR(exit 5) and never reaches the client.test rerunalone; it is being deprecated. The other related commands are optional if the change stays small.Comment
/assignto claim it.grzdev commentedon Oct 20, 2026
/assign
testsprite-hob commentedon Oct 20, 2026
Assigned to @grzdev. Thanks for taking this on. (0 more slots available.)
Your open assigned issues (3/3):
schedule update --env <name> --dry-runalways fails with UNSUPPORTED (exit 7) and says the server updated the schedule #377 — [Hackathon] fix(schedule):schedule update --env <name> --dry-runalways fails with UNSUPPORTED (exit 7) and says the server updated the scheduleruili-testsprite commentedon Oct 20, 2026
Scope update, replacing the per-command approach above. Please fix it once where arguments are parsed, so no command (current or future) can miss the check.
preActionhook insrc/index.ts. By the time it runs, Commander 12 has already parsed positionals intoactionCommand.processedArgs(variadic arguments arrive as arrays), andregisteredArgumentsgives you their names for the message.VALIDATION_ERROR(exit 5) before a client is created, both live and under--dry-run.src/liband unit-test it with a throwaway Commander program;src/index.tsis excluded from coverage.argumentkind tolocalValidationError, so the message namesArgument `<test-id>`instead ofFlag `--test-id`.requireNonEmptycalls on positionals intest.ts, the empty-id check inproject delete, and the one intest flaky. KeeprequireNonEmptyfor flags,requireDocsProjectId(it covers--projectand the env var), and the artifact-directory path guard.agent install ""currently drops the blank and installs the default target; after this it exits 5.--dry-run, including one blank value inside a variadic list.This also covers #381, #385, #395, #466 and #468, which are being closed as duplicates of this issue.