feat: sp — an AI-friendly CLI for investigating CI failures - #1
Open
gaurav02081 wants to merge 10 commits into
Open
feat: sp — an AI-friendly CLI for investigating CI failures#1gaurav02081 wants to merge 10 commits into
gaurav02081 wants to merge 10 commits into
Conversation
sp lets a developer or an AI agent investigate CI runs end-to-end from the terminal (no web frontend): list/show runs, summaries, classified failures, expected-vs-actual diffs, logs, errors, artifacts, samples, regression tests, auth, and a one-shot `investigate` triage command. Output defaults to JSON for agents, with a -o table human view. The API address is configurable via --base-url / SP_BASE_URL. Includes a test suite (pytest), lint/type configs (pycodestyle, pydocstyle, isort, mypy), and a GitHub Actions CI workflow.
Every enumeration the REST API validates is now restated in sp_cli/constants.py and wired into the commands as click.Choice, so a bad --status or --platform fails instantly as a usage error instead of costing a round trip and coming back as an HTTP 400. The values are not invented; each one mirrors a specific validator in the merged mod_api blueprint, and the module says which. The ones that are easy to guess wrong: - /runs?status= only accepts queued|running|canceled. It is derived from the latest TestProgress row, so pass/fail -- which are per-sample outcomes, not run states -- are deliberately absent. Use `sp run summary` for those. - _VALID_SAMPLE_STATUSES is pass|fail|missing_output|not_started. No 'skipped', no 'running'. - /regression-tests?active is a two-way switch, not tri-state: omitting it lists active tests only, so --active/--all was wrong and is now --active/--inactive. - There are seven token scopes, not six: system:write is separate from system:read so a monitoring token cannot reconfigure the platform. Also fills in filters the API supports but the CLI was not exposing (sample --sha256/--status, queue --limit/--offset, run --repository/--sort and the date window). ApiContractTests pins all of this, so a validator changing upstream fails here loudly rather than in production.
The classification codes say what broke, but not whether it is new. This adds a
cross-run verdict to every failure so the first question after a red build --
"did I break this?" -- is answered without opening the web UI.
Verdicts: NEW_REGRESSION (passed in the previous run, start reading here),
STILL_FAILING, NEVER_PASSED, FLAKY (two or more pass/fail flips), NO_HISTORY,
UNKNOWN. --history-depth N implies the flag.
The filtering in history.split_history is the part worth reviewing: the
/samples/{id}/history endpoint returns every regression test for a sample and
includes the run being investigated, so both have to be stripped before any
verdict is inferred. Leaving the current run in makes everything look
STILL_FAILING; leaving sibling tests in makes unrelated failures look like
history for this one.
Costs one lookup per distinct media sample, filtered to the run's platform and
cached across regression tests that share a sample -- so a 200-test run with 30
distinct failing samples is 30 calls, not 200.
sample-platform#1135 (errors, logs) and #1141 (test artifacts) are merged, so
the three commands that shipped as PENDING stubs are now real, and the
endpoints they left uncovered get commands of their own.
sp run error-summary <id> grouped counts -- the cheapest first look
sp run errors <id> per-test errors, --type/--severity/--sample
sp run infra-errors <id> VM / checkout / build / worker / storage
sp run logs <id> build log, --level/--source/--contains
sp run artifacts <id> binary, coredump, outputs, build log
Three contract details that shaped this:
- /runs/{id}/logs is the only cursor-paginated endpoint; sending it an offset
is a 400 ("Cannot mix cursor and offset pagination"). Hence
client.get_cursor_paginated alongside get_paginated, and --cursor/--all
rather than --offset. The table footer reported next_offset and silently
dropped next_cursor, so a partial log looked complete.
- A missing log answers 404 with code log_not_found, distinct from a missing
run's not_found, and points at the artifacts endpoint. Both map to exit
code 4, so callers must branch on error.code, not the status.
- The old --type help advertised 'test_failure', which the API cannot emit.
Test errors are derived per request and are only ever exit_code_mismatch,
missing_output, or diff_mismatch.
/runs/{id}/samples/{sid}/logs deliberately gets no command: it is a permanent
404 by design, because the CI worker does not store per-sample logs.
Covers every remaining merged endpoint, so the CLI is no longer read-mostly: sp run create POST /runs -- queue a run sp regression show/create/edit/rm sp category ls/create/edit/rm sp sample details upload record, extra files, media info sp auth whoami/users/set-role sp admin maintenance/pause/resume sp admin blocked-users ls/add/rm sp admin forbidden-extensions ls/add/rm 409 conflict now maps to its own exit code (8). A refused delete is not a validation failure -- the body was fine, the world disagreed -- and callers need to tell "this test still has 23 results" apart from "your request was malformed". Both delete paths return it, and both suggest the alternative: retire with `edit --inactive` rather than deleting. Request-shape details that are easy to get wrong, all pinned by tests: - Regression-test categories are given by name, not id, must already exist, and on PATCH the list is replaced rather than merged. - A new regression test is created inactive unless --active, matching the API's default: a maintainer should see what it produces on a verification run before it joins the suite. So `active` is sent only when asked for, otherwise "off" is indistinguishable from "unset". - POST /runs needs a full 40-char commit_sha and owner/repo -- both rejected locally -- and schedules a test of an existing CI artifact rather than triggering a compile. - Forbidden extensions are stored lower-cased without a leading dot, so .MKV and mkv normalize to the same request. - Blocked users key on the numeric GitHub id, not the login: logins can be changed and reused, which would silently unblock somebody. PATCH bodies are sparse -- only the flags you pass are sent -- and an empty edit is a usage error rather than a pointless round trip.
Three human-facing conveniences, all bound by the same rule: machine output stays clean. Every one of them is suppressed when the output is not a live terminal, and none of them can reach stdout in JSON mode. Saved session (sp_cli/config.py) `sp auth login` writes the token to ~/.config/sp/config.json so it does not have to be pasted into SP_API_TOKEN for every shell. Precedence is --token > SP_API_TOKEN > the file, so an explicit credential always wins. --no-save opts out for shared machines. The file is created through os.open at mode 0600 rather than chmod-ed afterwards, so the token is never briefly world-readable, and a warning is printed if an existing file has looser permissions. A corrupt file degrades to "logged out" instead of breaking every command, and `sp auth logout` clears it even when the server call fails -- an already-expired token must not be left behind on disk. Colour (sp_cli/output.py) The code and verdict columns are colorized by severity. Padding is applied before styling, because escape codes have no display width and colorizing first pushes every later column out of line. Gated on table mode, a TTY, NO_COLOR, and --no-color. Spinner (sp_cli/progress.py) Shown during the multi-page calls -- run failures, run logs --all, investigate -- and drawn on stderr, self-erasing, so even when it does run it cannot contaminate a payload being parsed on stdout. `sp shell`, the fourth item on the backlog, is dropped rather than built: a REPL is a second interface every future command must work in, it loses the pipes that make the CLI useful, and the primary consumer is an agent driving one-shot commands.
The README still showed only the handful of commands the first draft had, and described `sp run logs` as "raw run logs" from when it was a stub. Groups the commands by what you are actually doing -- investigating a failure, browsing, maintaining tests, administering -- rather than listing them flat, and documents `sp auth login` as the way to authenticate now that the token is saved. Adds the exit-code table. Scripts and agents branch on these, so they belong in the README rather than only in the client docstring.
Both of these are things a live run against production surfaced that the
mocked tests could not.
Retry
`investigate --with-history` makes one call per failing sample. On a real
run -- 45 failures, ~1.5s per request -- that is a 60-90 second window, and a
single 30s read timeout partway through aborted the whole command with exit
3, discarding every lookup already done. That is exactly what happened.
Failed GETs are now retried with exponential backoff plus jitter, for
connection errors, read timeouts, 429 and 5xx. Retry-After is honoured when
the server sends it, but clamped to 30s so a header of 3600 cannot hang the
CLI. Jitter is there so a fleet of agents riding out the same blip does not
resynchronise into a second thundering herd.
Only GET is retried. POST /runs is not idempotent -- a retry racing a
slow-but-successful first attempt would queue the run twice -- and a repeated
DELETE turns a success into a confusing 404. 4xx other than 429 is
deterministic, so asking again just wastes a round trip.
Notices go to stderr, never stdout, so JSON stays parseable. --retries 0
restores the previous fail-fast behaviour.
--decode
The output endpoint returns the file base64-encoded inside a JSON envelope,
so `sp run output` printed a multi-kilobyte blob that no one can read and no
diff tool can consume. --decode writes the decoded bytes to stdout instead:
sp run output 9388 11 --decode > actual.srt
Written to sys.stdout.buffer rather than echoed, because these are subtitle
files carrying CRLF and sometimes non-UTF-8 bytes -- re-encoding them would
corrupt the very diff you are trying to read. Verified against production:
5562 bytes out, CRLF intact.
Two existing client tests now pass retries=0. They assert the exit-code
mapping, not the backoff, and 429 is retryable -- leaving the default on made
them sleep for seconds.
Verified against production: /samples/{id}/history returns 504 there, and one
failed lookup aborted the entire investigation with exit 3 -- discarding the
run summary and all 45 classified failures to report a single missing verdict.
The classification is the bulk of the answer and it was already in hand.
A failed lookup now marks that row UNKNOWN and carries on, and the report gains
a `history_incomplete` block naming the samples that could not be fetched, so
the gap is visible rather than silently absent.
After three consecutive failures the endpoint is treated as down and the
remaining rows are marked without calling it. Without that circuit breaker a
broken endpoint costs one full timeout per failing sample -- 45 samples at 30s
is a 22-minute hang for an answer that is not coming.
Run 9388 on production now completes in ~1.5 minutes with all 45 failures
classified (24 MISSING_OUTPUT, 20 OUTPUT_DIFF, 1 SEGFAULT), where before it
returned nothing at all.
Note this is damage control, not a fix for the underlying problem: the verdicts
are all UNKNOWN on production because the endpoint paginates in Python after
loading every result for the sample across every run, so ?limit=5 does not
reduce the work. That needs fixing in sample-platform.
gaurav02081
force-pushed
the
feat/initial-cli
branch
from
August 5, 2026 20:36
843365d to
8db09b1
Compare
gaurav02081
changed the title
gaurav02081
changed the title
Author
|
@cfsmp3 @canihavesomecoffee ready for review whenever you have time.
I tested it against the live deployment on a real PR run (9388, linux, commit
Two findings worth your attention:
The PR is large because it is the whole tool, but the 10 commits are each self-contained and green, so reviewing commit by commit is much easier than the combined diff. I can split it into ~6 smaller PRs if you would prefer — the branches are already prepared. |
A high-effort review of the branch confirmed ten defects. They cluster in three areas, and several were in code written an hour earlier -- completing a production run was not evidence it was correct. Credential handling Running `pytest` overwrote the developer's real ~/.config/sp/config.json: one login test omitted --no-save and CliCommandTests has no XDG isolation. It had already fired on this machine, leaving the literal `spci_x` behind. Adding --no-save to that one test is not the fix -- relying on every future test remembering it is what failed. tests/conftest.py now redirects XDG_CONFIG_HOME and HOME for every test automatically. `auth logout` cleared the saved session unconditionally, so revoking a scratch token from SP_API_TOKEN deleted an unrelated 30-day credential that cannot be recovered. It now compares the effective token against the saved one by value. The error path splits too: 401/403 still clears, because the token is dead either way, but a dropped connection leaves it alone rather than discarding a token that is probably fine. save_token relied on os.open's mode, which is ignored for a file that already exists -- so re-login wrote a fresh token into a still-0644 file and chmod-ed it afterwards. It now fchmods the descriptor before any byte is written. investigate --with-history Three bugs that only appear with more than one failing regression test per sample, which is why the original tests missed them: a failed lookup was never memoised, so it was re-called and re-counted for every row sharing that sample and one bad sample tripped the breaker; the given_up check preceded the cache check, discarding history already fetched successfully; and --history-depth was unbounded although the API rejects limit > 100, so --history-depth 100 silently disabled all history while still exiting 0. The window itself was also wrong, and only half of that is fixable here. The endpoint pages over *all* of a sample's regression tests and slices before the CLI can filter, so a page of N gave roughly N/(tests on the sample) runs of the test under investigation -- reporting NEVER_PASSED for a test that passed nine runs ago. The CLI now requests a full page and applies the depth after filtering, and when a saturated page still leaves a short window it sets window_truncated and drops NEVER_PASSED to low confidence: "never passed" and "did not pass in the few runs I could see" are different claims. A real fix needs sample-platform#1161, and the docs now say so. Output contracts --decode ignored the envelope's truncated flag, writing a file that ends mid-stream and looks complete -- every diff against it reports spurious missing lines. It is now refused, naming download_url, with --allow-truncated as an explicit opt-in. `auth revoke` and `auth logout` printed English sentences to stdout, so piping either into jq failed while every other command worked. Both now render JSON. LOG_MAX_LIMIT mirrored a 1-500 clamp that sits behind a validator rejecting anything over 100, so it was unreachable and the help advertised a page size the API refuses. Deleted rather than corrected: it duplicated a rule that is uniform across both paginators. That audit found 33 other unvalidated --limit/--offset options, now all bounded by IntRange. The subtle fixes were revert-checked rather than trusted: restoring the old code makes the new tests fail, including "token written while mode was 0o644". 182 tests, up from 166.
gaurav02081
force-pushed
the
feat/initial-cli
branch
from
August 6, 2026 08:03
843365d to
8d2ddc6
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
spis a command-line client for the Sample Platform REST API. It exists so a developer — or an AI agent — can investigate a CI failure end to end from the terminal, without the web frontend. Output is JSON by default so it can be piped intojqor driven by a script, with-o tablefor humans.This is a new repository, so this PR is the whole tool. It is large, but the 10 commits are each self-contained and green, and GitHub will show them individually — reviewing commit by commit is much easier than reading the combined diff, and the order below is the order they were built in.
If you would rather have this as a series of smaller PRs, say so and I will split it — the branches are already prepared. It is not stacked here only because chaining PRs needs the base branches to exist on
CCExtractor/sp_cli, which I cannot push to.Reading guide
feat: sp, a command-line client…feat: mirror the API's validators…constants.py— every enum the API validates, as Click choicesfeat: sp investigate --with-historyfeat: run errors, logs, artifacts…feat: run create, regression/category CRUD…feat: saved login session, colorized codes…docs: document the full command surface…fix: retry transient GET failures…fix: degrade gracefully when a sample's history…fix: ten defects found by code reviewWhat to look at first
classifier.py/triage.py— the reason this tool exists. Every failure gets a stable code (SEGFAULT,ABORT,TIMEOUT,EXIT_CODE_MISMATCH,MISSING_OUTPUT,OUTPUT_DIFF), so you get a straight answer about why a test failed without reading logs.constants.py— every enumeration the API validates, mirrored so bad input fails instantly as a usage error instead of costing a round trip. Each value cites the validator it mirrors. The ones easy to get wrong:/runs?status=accepts onlyqueued|running|canceled(pass/fail are per-sample outcomes, not run states);/regression-tests?activeis a two-way switch with no "list everything"; and there are seven token scopes, not six —system:writeis separate fromsystem:read.history.py—split_historydoes filtering the endpoint does not./samples/{id}/historyreturns entries for every regression test on the sample and includes the run being investigated, so both must be stripped before any verdict means anything.client.py— GET is retried with backoff; POST and DELETE never are.POST /runsis not idempotent, and a repeated DELETE turns a success into a confusing 404.Testing
182 tests;
isort,pycodestyle,pydocstyle, andmypyclean at every commit, not just at the tip.Beyond unit tests, this was exercised two ways that changed the code:
mod_apifrom anupstream/masterworktree on a copy of the dev database. That found a real bug:system:writewas missing from the scope list, which silently made everysp adminwrite impossible.run outputhaving no way to get the actual file. It also confirmed the classifier on realSEGFAULT/OUTPUT_DIFF/MISSING_OUTPUTfailures — the dev database only ever produced one of those.Commit 10 is a code-review pass; the subtle fixes there were revert-checked, meaning the old code was restored to confirm the new tests actually fail against it.
Known limitation
investigate --with-historyreturnsUNKNOWNverdicts against production, becauseGET /samples/{id}/historytimes out there — CCExtractor/sample-platform#1161, filed with a diagnosis and a suggested fix. The CLI degrades gracefully rather than failing, andNEVER_PASSEDdrops to low confidence when the window was truncated. Everything else works against production today.