Add AI Test Authoring commands and run kind - #1100
slvinittomar wants to merge 12 commits into
Conversation
slvinittomar
added
enhancement
|
Manual test plan for this PR: specs/001-ai-test-authoring/manual-test-plan.md — rows tagged by priority and existing coverage; two known gaps (dynamic |
Manual test plan for the scenarios PR #1100 touches: the authoring command group, the `kind: authoring` runner, and the shared surfaces that changed (run dispatch, root command list, schema bundle). Rows are tagged with priority and existing coverage so testers can concentrate on what automation cannot reach — interactive prompts, the spinner, Ctrl-C mid-run — and on regression of the other kinds against the new bundle. Every expected value was checked against the code or a live-service observation while writing the plan. Two code follow-ups surfaced and are recorded as known gaps so they are not filed as new: dynamic shell completion for `testcases code --target` yields nothing because cobra runs no pre-run hooks during completion, and a 401 from the user lookup is reported as "the current user has no organisation" because the shared user client does not check the HTTP status. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
Took a pass through this. Overall really solid – the read/mutation retry split, the tri-state null handling on schedule updates and the delete confirmation flow are all nicely done. A few things I'd like to see addressed before this merges:
Smaller stuff (can be follow-ups): zerolog warnings go to stdout and break Happy to dig into any of these in more detail. |
|
Pushed three commits addressing the code review:
All four gates pass locally: |
|
Thanks — all nine were real; I verified each against the code before changing anything, and none had been fixed by the earlier round. Pushed as four commits. 1. Interrupt/timeout now stops the run ( Same commit fixes concurrency counting runs instead of jobs: each case is now weighted by the jobs it will start, capped at the budget so a case needing more than the whole budget still runs alone rather than never. Worth flagging that my first attempt at this deadlocked — two cases each took one of two slots and waited forever for the other's — so acquisition is serialised behind a gate. The concurrency test caught it, not review. 2. Failed generation in JSON mode ( 3. 4. Resource IDs scrubbed ( stdout logging is filed as #1101 rather than fixed here. The root cause is global ( All four gates pass locally: |
The build job in test.yml passed -X ldflags for github.com/saucelabs/saucectl/cli/version.*, a package that does not exist; Go ignores -X for unknown symbols, so CI binaries were never stamped. Point it at internal/version, which is what .goreleaser.yml uses. The schema target used pushd/popd, which are bash builtins and fail under a POSIX sh make shell. Use a plain cd inside the recipe's subshell instead; the effect is identical. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Adopt GitHub spec-kit (v1.0.4, skills mode) as a shared workflow: the .specify tooling (templates, scripts, workflow registry, integration manifests) and the Claude skills that drive it under .claude/skills, including the repository's own saucectl-dev skill, are committed together so anyone who clones the repository can run the same specify/plan/tasks/implement workflow. The constitution in .specify/memory records the conventions this repository already follows. Add the feature specification for AI Test Authoring under specs/001-ai-test-authoring: spec, plan, research, data model, quickstart, CLI and configuration contracts, requirements checklist and task list. The research document is the load-bearing artifact: every behaviour of the AI Authoring API it relies on was verified against the live service, and several contradict the published specification (Basic rather than bearer auth, a list endpoint that ignores its path parameter, an undocumented error.data[] field, an empty stored tunnel name that breaks runs unless explicitly nulled, schedule updates that require the full object). Each finding records how it was observed so it can be re-run. .specify/feature.json is machine-local and stays ignored via .specify/.gitignore. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Add a `saucectl authoring` command group covering the Sauce Labs AI Test Authoring service, and a `kind: authoring` configuration so authored suites run through `saucectl run` with the shared reporters, artifact download, concurrency limit and CI exit codes. Commands: testcases (list, get, delete, rename, run, list-runs, get-run, list-tags, generate, generate-status, code, list-code-targets), testsuites (list, get, create, update, delete, run), schedules (list, get, create, update, enable, disable, delete), variables (list, get, create, update, delete) and download-artifact. Every listing offers text and JSON output and reports the total; every delete confirms what it affects and refuses when non-interactive without --yes. Behaviours verified against the live service and encoded here, several of which contradict the published API specification: - HTTP Basic auth, not bearer. - 404 is an ordinary answer for this API and is never retried; mutations are never retried at all, so a lost response cannot start a second run or authoring session. - Run listings filter on the testCaseId query parameter; the path parameter alone returns the whole organisation's runs. - A run has no status field: completion is inferred from per-job success, which appears on the run resource before the Sauce job completes. - Some stored test cases carry an empty tunnel name that makes runs fail with SC_TUNNEL_NOT_FOUND; the runner sends an explicit null. - Authored runs publish no junit.xml, so JUnit content is synthesised per job. - The entitlement gate uses the platform entitlements API and distinguishes "not in your plan" from "could not verify". - Schedule updates require the complete object; omitted optional fields are kept and an explicit null clears them. UTC is not a valid timezone. Configuration validation warns about every shared setting this kind cannot honour rather than dropping it silently. The framework schema is added and the bundle regenerated; local validation of this kind reports an advisory error until the bundle is published from main. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Manual test plan for the scenarios PR #1100 touches: the authoring command group, the `kind: authoring` runner, and the shared surfaces that changed (run dispatch, root command list, schema bundle). Rows are tagged with priority and existing coverage so testers can concentrate on what automation cannot reach — interactive prompts, the spinner, Ctrl-C mid-run — and on regression of the other kinds against the new bundle. Every expected value was checked against the code or a live-service observation while writing the plan. Two code follow-ups surfaced and are recorded as known gaps so they are not filed as new: dynamic shell completion for `testcases code --target` yields nothing because cobra runs no pre-run hooks during completion, and a 401 from the user lookup is reported as "the current user has no organisation" because the shared user client does not check the HTTP status. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
A review of this branch turned up fifteen behavioural defects; two were reproduced with throwaway probes. None were caught by lint or the tests, so each fix carries a regression test. Contradictions between what we report and what we return: - An accepted --async run whose jobs the service has not listed yet was reported as failed, with a synthesized JUnit failure, while the process exited 0. A pipeline gating on the report failed a launch that worked. Only a genuine start failure is a failure now. - testcases get --revision was resolved, validated, and then discarded under -o json, so a script pinning an older revision silently received the latest. The JSON document is now narrowed to the pinned revision. - The shell-history warning for variables update --value read the command flag rather than the variable's stored secrecy, so it stayed silent in the one case it exists for: rotating an existing secret. Waiting and polling: - Every generation poll failure was reported as "generation is still running", telling a user with a mistyped task id to keep polling something that does not exist, and the cause was joined with %v so no caller could match it. Transient failures are now tolerated as the run poller already tolerated them, a persistent failure is reported as itself, and the cause is wrapped. The tolerance is bounded so nothing waits for ever even without a caller deadline. - pollRun polled with the testCaseId from the start response. If that field were ever absent it would 404, which the poller treats as propagation lag, burning the whole 30-minute suite timeout in silence. It falls back to the identifier we asked with. - Tunnel readiness was checked with sauce.tunnel.owner while the run sends only the name, so the check could pass against a colleague's tunnel and every run then fail with SC_TUNNEL_NOT_FOUND. Validation now checks exactly what the run will send. Byte versus character limits, which the service states in characters: - The build name was truncated by slicing bytes, which split a rune and put invalid UTF-8 in the run request while dropping characters that were inside the limit. - Generation bounds for --name, the intent, --test-url and each tag rejected valid non-ASCII input for the same reason. Silence where the tool cannot honour a setting (Constitution VIII): - --all discarded both --skip and --limit. --skip is now applied as the starting offset and --limit warns that whole pages are fetched. Other correctness and hygiene: - A failed artifact download left a truncated file behind, which the command's own overwrite guard then refused to replace on the obvious retry. Downloads are written beside the destination and renamed. - The config schema forbade isRdc on a target while the Go type and --target-json both accept it, so the same target shape worked on one surface and was rejected on the other. The schema accepts it, and a run request carries capabilities only, as the API documents. - Confirmation-prompt lookups were issued even under --yes, where no prompt is printed, adding a wasted round trip per asset in scripts. - jobURL and the capability extraction were duplicated across two packages; both now live in internal/authoring so the run results table and the CLI tables cannot describe the same job or target differently. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
The first version was a set of tables with a Coverage column recording whether each row was already unit-tested or had been exercised once against the live service. That framing was wrong for the document's reader: a manual test plan is executed in full, and any column that looks like permission to skip is read as permission to skip. It is now 114 numbered scenarios in dependency order, each with exact commands and an Expected list of observable results — strings copied from the source, exit codes, files, counts. Assets are created before the scenarios that use them and deleted in the confirmation-matrix part, so teardown is part of the plan and doubles as verification of the destructive-action safeguards. Environment-dependent scenarios sit in their own part with instructions to record "not run" and why. Writing it this way surfaced two real defects, recorded as known gaps so testers recognise them rather than filing them again. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Captures how the AI Test Authoring plan was produced, so the next one starts where this one ended rather than repeating the same corrections. The skill takes a pull request, branch or commit range, scopes the work from the diff by separating new surface from shared surface, enumerates the real command surface from the binary rather than the spec, and settles any claim about the remote service with a read-only probe before it goes in the document. Three reference files carry the document template, the repository's own conventions, and the review checklist that turned the first draft into a correct plan. Two rules earn their place from experience. Quoted messages must come from the source or an observed run, because a message quoted from memory is the most common way a plan lies. And the review loop repeats until a full pass finds nothing: the plan this skill is derived from needed several passes, the last of which still found six wrong jq paths and a fixture path that does not exist. evals/evals.json holds the test prompts used to validate the skill against a no-skill baseline, so a future change can be measured rather than assumed. On the first run it scored 92.5% against 71.8%, with the separation coming from structure and message verification rather than from finding more. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
An interrupted or timed-out run kept a VM or device busy for its full duration. Every other kind stops the job in this situation, and the job service already wired in for authoring was stop-capable — only its download side was being used. Stopping runs on a context detached from the cancelled one, mirroring the localCtx in saucecloud whose comment says it exists so jobs are not left abandoned, with a deadline of its own so nothing waits for ever. Errors are ignored as they are elsewhere: a job may already have ended, or be in a state that cannot be stopped, and either way there is nothing to do about it. The log messages no longer claim the run continues. The concurrency limit now counts Sauce jobs rather than runs. One run fans out to one job per target, so a per-run semaphore let a four-target suite put four times the configured load on the organisation's capacity, which SC-011 exists to prevent. Each case is weighted by the jobs it will start, capped at the whole budget so a case needing more than that still runs — alone — rather than never. Slot acquisition is serialised behind a gate. Collecting slots concurrently would let two cases each hold part of the budget and wait for ever for the other's, which is a deadlock rather than merely unfair; the first version of this change had exactly that bug and the concurrency test caught it. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Four unrelated defects in the authoring commands, grouped because their regression tests share a file. A failed generation exited 0 under -o json. The JSON branch returned before the status switch, so a FAILED task printed its payload and succeeded while text mode correctly exited 1 — which breaks CI gating for anyone asking for machine-readable output. The payload still comes first, because a script asked for it, but the task's own status decides the exit code in both formats. A wait that ends without the task finishing now always carries the task id, in the error and in the emitted JSON object, so it can be reattached without parsing prose. The two ways a wait can end are answered differently on purpose: an interrupt means the user stopped us and the task is most likely still running, so that is reported with the reattach hint; our own deadline expiring while every poll keeps failing means the task's state is unknown and the polling is broken, so the failure is reported instead. Advising more polling of something broken was the defect this path was reported for. Both exits route through one decision, because an earlier branch used to return the context error directly and bypass it. --timezone UTC and Etc/UTC are refused before the round trip. Neither appears in the service's accepted list, which holds region and city zones only; anything else is still left to the service, which holds the real list. Setting and clearing the same schedule field in one command was resolved silently in favour of the unset, because that loop ran last. It is now a conflict naming both flags. The large-listing warning is held back under -o json. saucectl's logger writes to stdout for every command group, so a warning emitted while rendering a document lands inside it and breaks jq. The global behaviour affects every command and changing it would alter output users already parse, so it is filed separately rather than changed here. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
RunTestSuite always sent an explicit "buildName": null, unlike RunOptions on the test-case run endpoint, which omits the same field. Nothing asked for the null and no test covered the request body. Verified on a throwaway suite with no members, so no jobs were queued and no VM time was spent: an omitted buildName, an explicit null and a real name all reach the same business error, TEST_SUITE_NO_RUN_JOBS, rather than an INVALID_BODY. Omission is therefore valid and the null was unnecessary. Unlike scTunnelName, where a null clears a stored value that would otherwise break the run, no null is load-bearing here. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
The committed fixture pointed at one specific private test case, and the plan, specs and config contract carried identifiers belonging to colleagues' accounts. They are opaque and useless without credentials to that organisation, but this is a public repository and the fixture was also unusable by anyone outside it. The fixture now carries a placeholder and explains why: authored test cases live in the service rather than the repository, so no identifier can work for every reader. The manual test plan finds a case with a stored empty tunnel name through a read-only query instead of naming one. The specs keep a truncated form where the identifier is evidence for a recorded observation, so the trail still reads without a full id landing in history. The plan's expectations are also brought in line with the behaviour changed in this round: an interrupt or timeout now stops the run rather than leaving it going, and a new scenario covers --all honouring --skip while warning that it cannot honour --limit. The suite-run probe is recorded in research.md alongside the other observations. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
The authoring framework schema $refs the shared sauce subschema, so the region enum added in #1102 must be reflected in the generated bundle. Keeps the check-schema CI diff empty. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
aed6d01 to
56f9a6e
Compare
|
The Spec Kit toolchain, the Claude Code skills and The branch has been rebuilt from current The two PRs touch disjoint files and neither needs the other to build. Merging this one first is easier on the reviewer of #1104, whose test plan describes the commands added here. Note that my earlier link to |
|
Replaced by two PRs, rather than force-pushing over this branch:
This PR can be closed once you have had a look at those; leaving it open for now so the review conversation here stays easy to find. |
2 participants
Summary
Adds Sauce Labs AI Test Authoring to saucectl:
saucectl authoringcommand group covering the AI Authoring API — test cases, test suites, schedules, variables and artifact download (31 commands, text and JSON output everywhere);kind: authoringconfiguration so authored suites run throughsaucectl runwith the shared reporters, artifact download, concurrency limit and CI exit codes.The branch carries three commits: a small CI/Makefile fix (the build job stamped a non-existent
cli/versionpackage; the schema target used bash-onlypushd), the spec-kit toolchain plus the feature specification underspecs/001-ai-test-authoring/, and the implementation.Behaviours verified against the live service
Several contradict the published API specification. Each is recorded with how it was observed in
specs/001-ai-test-authoring/research.md, and each has a code comment where it matters.GET /testcases/{id}/runsignores its path parametertestCaseIdalways sent as a query parameter; without it the whole org's runs come backsuccessappears on the run resource ~19 s in, before the Sauce job completes*bool, run resource polled every 5 sscTunnelName: "", which fails runs withSC_TUNNEL_NOT_FOUNDnullwhen no tunnel is configuredjunit.xmlreporters.junitis not a set of empty containerserror.data[]with the actionable messagenullclears--unsetsendsnull;UTCis not a valid timezone so--timezoneis requiredTesting
make lint,make test,make build,make schemaall pass; the bundle is byte-identical to a fresh regeneration.Notes for reviewers
config.ValidateSchemavalidates against the bundle onmain, sokind: authoringprints an advisory schema error locally until this merges. It fails soft, like every other new kind before it.storage delete; the justification is inplan.md(Complexity Tracking): these are shared org-level assets with no undo.--yeskeeps it scriptable and a non-interactive run without--yesrefuses rather than hangs.List[T],ListAll) are new to this repository; flagged in the plan.saucelabs/sauce-docsand is not yet drafted.🤖 Generated with Claude Code