Skip to content

[CLI] Fix review edge cases, and release each merge to main - #18

Merged
ilyazub merged 28 commits into
mainfrom
ilyazub-serpapi-cli-refinement
Oct 13, 2026
Merged

ilyazub merged 28 commits into
mainfrom
ilyazub-serpapi-cli-refinement

Conversation

@ilyazub

@ilyazub ilyazub commented Oct 13, 2026

Copy link
Copy Markdown
Contributor

Opus 5.5 Max wrote this:
Fix the edge cases that model reviews found on main before v0.3.0, and release each merge to main automatically once its tests pass. login could save the API key in the current directory or write it into another file. Ctrl-C could drop results that were already billed, or leave the terminal without echo. And merged fixes reached nobody until someone pushed a tag. Merged with the release:minor label, this PR releases v0.3.0. Follow-up of #17.

Main 7acfb80 is #17's merge and the planned v0.3.0. GPT-5.6 Sol Max and Claude Opus 5.5 Max reviewed it for edge cases, and GPT-5.6 Sol Max then reviewed the fixes twice. Each fix started as a failing test. The table runs the real binaries against a local fake SerpApi.

main 7acfb80 this PR de6215b
login with HOME unset or HOME=. key saved under the current directory exit 2, nothing saved
login with config.toml.tmp symlinked to another file key written into that file that file untouched
login with a stale 0644 config.toml.tmp config saved 0644 0600
Ctrl-C or SIGTERM at the hidden key prompt exit 130 after 3.1 s, echo off at once, echo on
Ctrl-D at the hidden key prompt still waiting after 20 s exit 2, "No API key given"
Ctrl-C on page 4 of --all-pages --jq nothing printed --jq result for pages 1–3
serpapi help nope exit 0, usage as plain text on stderr exit 2, one JSON line
hint on a large no_cache=true --all-pages result the repeat "comes from the cache" "a new, counted search for each page"
hint on a large zero_trace=true result read the stored search for free the search isn't stored
Control: main against itself, 120 commands 0 differ

Against main, 2 of the 120 matrix commands differ, --help and -h, which now say what zero_trace costs and that exit 130 means the run stopped before it finished. stderr stays JSON-only in all 120.

  • Raw mode at the hidden prompt. The caller now sets and undoes it, so a reader that a signal leaves behind can't turn echo off again. With the reader forced to start after the restore, the earlier layout left echo off in 5 of 5 SIGTERM runs and this one in 0 of 5. Ctrl-C there is a key, so it stops the run as SIGINT would, even when typed with Enter in one read; nothing is saved. Cost: Ctrl-Z no longer suspends at that prompt.
  • After a signal, --jq stops 2.25 s in. That is a quarter of the 3 s grace before the forced exit, both counted from the signal. Main doesn't run --jq on fetched pages at all. An earlier commit here let the forced exit fire first, and in 1 of 3 runs both printed an error line; now it's 1 line in 3 of 3.
  • A signal after everything printed still exits 0. The output is complete, and 130 would tell a script to retry billed searches.
  • ZeroTrace comes from server code, not a live call. serpapi/SerpApi's api_controller.rb sets no_cache = to_bool(zero_trace || params[:no_cache]), and its spec checks that a ZeroTrace search isn't saved. The hint reads both flags as the pinned Mongoid 7.5.4 does. A value in neither of Mongoid's true and false lists gets "may", unless the other flag settles it.
  • Four commits can fail alone. 92590fb and 7ae9a22 fail go test -race on a test-only race that 81d84dd fixes. 46af5c5 and 54815f5 carry another, seen in 2 of 4 runs, that f849cf4 fixes. 7ae9a22 also dropped help completion, which 803cda6 restores. Squash-merge, or review commit by commit with that in mind.

Not covered: Windows compiles and vets, but the raw-mode prompt never ran on a Windows console. Three pre-existing gaps are left for later:

  • a config directory that others can write isn't tightened;
  • a failed save that can't remove its temp file doesn't say so;
  • with stdin on a terminal and stderr redirected, the prompt goes into stderr.

Merges to main now release. v0.2.3 shipped on 2026-03-31; #7, #9, #15 and #17 merged after it, and none reached users. After both test legs and the live e2e pass on main, CI tags the merge commit with the next version and runs release.yml. The version is a patch, or a minor or major when any pull request merged since the last release has that release:* label. release:skip, or a merge where nothing that ships changed, releases nothing.

Merging releases
this PR, labeled release:minor v0.3.0, with .github/release-notes/v0.3.0.md as its notes
this PR, unlabeled v0.2.4
a later PR that changes only docs, tests or CI nothing

That table comes from dry runs of release-tag.sh on this repo's history, which made 7 read-only API lookups, and a 27-case harness that now runs in CI before any tag. The "Release preview" check shows the same for each PR.

  • Guards. Outside pull requests a skipped live test fails CI. The tag must point at the tested commit. An existing tag, or one older than the newest release, is refused. One locked job builds from the exact tag, uploads and updates Homebrew, and runs on main wait their turn (queue: max). A published version is never rebuilt with a newer Go: a complete release only refreshes Homebrew, and a partial one is fixed forward with the next version.
  • Labels: release:minor, release:major and release:skip now exist, and this PR has release:minor. Before relying on it, maintainers should decide on a ruleset that blocks updating or deleting v* tags, and on immutable releases: today neither is set, so someone with write access could move a tag after CI checked it.
  • Not run yet: the tag and release jobs first run for real on this merge. GoReleaser 2.18.3 ran with the same flags on 8f1c581 with --skip=publish: 5 archives, checksums 5 of 5. If publishing fails before any upload, re-run it; after a partial upload, release the next version.

Merging releases: with release:minor, CI tags the merge commit v0.3.0 and publishes it. Then check the 5 archives, the tap's serpapi-cli 0.3.0 commit and brew test serpapi-cli, as AGENTS.md says.

Checks and reviews

Opus 5.5 Max checks on de6215b (macOS arm64, fake SerpApi on localhost, base main 7acfb80); the 11 commits after it change only CI and release files, and check.sh passes again on 8f1c581:

check.sh                gofmt, vet, staticcheck ok; 322 package tests passed;
                        tests/: 2 passed, 10 skipped (live, no key)
go1.25.0 -race ./...    324 passed, 10 skipped, 0 failed
-race, 20 times         pkg/cmd and pkg/config: 3,800 runs, 0 failures
each of the 17 commits  vet, staticcheck, go test -race ./pkg/... alone: 15 pass;
                        92590fb and 7ae9a22 fail on the test-only race above
windows/amd64, linux    vet ok; 11 of 11 test binaries compile
matrix, 120 commands    main vs main: 0 differ; main vs this PR: 2 (--help, -h)
PTY probes              hidden prompt: key, backspace, ^C at once and after 0.3 s,
                        Enter and ^C in one write, ^D, SIGTERM, ^C with stderr a
                        full pipe; [y/N] prompt: ^C
release-tag.sh          shellcheck clean; 27-case harness with a stub gh; dry runs
                        on this history, 7 read-only API lookups
workflows               actionlint clean but for queue: max, which it doesn't know;
                        e2e skip guard run locally: no key, exit 1
goreleaser 2.18.3       8f1c581, --skip=publish, flags as in release.yml: 5
                        archives, checksums 5/5, formula test 5/5, govulncheck 0

Reviews: GPT-5.6 Sol Max (main, then this series in two passes; the last confirmed all 5 of its second-pass findings closed) and Claude Opus 5.5 Max (main). GPT-6 Astra advised (effort not exposed). Found and left for the roadmap: completion bogus exits 0, and version and --version differ when SERPAPI_TIMEOUT is invalid.

ilyazub and others added 28 commits October 13, 2026 00:40
login wrote the key through config.toml.tmp, a fixed name: a 0644 file
left there made the saved key world-readable, a symlink there sent the
key to its target, and 7 of 8 concurrent logins failed. Without HOME, the
key was saved to and read from ./.config/serpapi in the current
directory. The temp file is now created with O_EXCL and mode 0600, and
with no config directory login says to set HOME instead of saving.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Ctrl-C or SIGTERM during --all-pages --jq printed nothing: --jq ran with
the canceled context, so it stopped before its first step, and the pages
already fetched, each a counted search, were lost. README and --help
promise them. --jq now runs on them for at most the 3 s grace period
after the signal, then the run exits 130 as without --jq.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A large result without a search id, such as merged --all-pages pages,
suggested repeating the command with --jq because "the same search within
an hour comes from the cache". With no_cache=true the repeat is a new,
counted search, for every page with --all-pages; an agent following the
hint paid again. The hint now says so.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Since #17's signal handling, Ctrl-C at login's hidden key prompt waited
the 3 s grace period, said "the output may be cut off", and left the
terminal not echoing; the [y/N] prompt and a key piped through an open
stdin waited too. In a pseudo-terminal, both prompts now exit 130 at
once, the hidden one with echo restored, and nothing is saved.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
serpapi help serach printed "Unknown help topic" and the whole usage as
plain text on stderr and exited 0, though exit 2 means the command is
wrong and stderr carries JSON lines. It now exits 2 with one usage_error
line that names the nearest command, or points to serpapi --help when
none is close; help with a known topic or none prints as before.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The read goroutine looked up the command's stdin itself, and after an
interrupt nothing waits for it, so go test -race flagged that lookup
against the next test's SetIn (go1.27.2 and go1.25.0). The caller now
builds the reader. The binary exits at that point, so only tests could
race; -race over pkg/cmd: 20 of 20 runs pass.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
7ae9a22 replaced cobra's help command, which dropped its completion
(serpapi help s<TAB> offered files instead of search) and its long
help text. cobra's own help command is back; only its run step is
replaced, so an unknown topic still exits 2 with one JSON line.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
1f7423f stopped using the current directory when HOME is unset, but
HOME=. or any relative HOME still put the key under, and read it from,
wherever the command ran. Dir now accepts only absolute paths, so
login stops with exit 2 and search, account and archive ignore any
config file there; the error names %AppData% on Windows.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A large ZeroTrace result was told to re-read its stored search for free,
but ZeroTrace stores nothing and skips the cache (serpapi/SerpApi
api_controller.rb: no_cache = to_bool(zero_trace || no_cache)). The hint
now says the repeat is a new, counted search. It reads both flags the
way the API does through Mongoid 7.5's Boolean.evolve: no_cache=False is
off and served from the cache; a word in neither of Mongoid's lists gets
"may", since Mongoid 8 takes it as true.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
e0af7a7 ran --jq on the pages fetched before a signal only when a page
was still loading. A signal after the last page or a single response
arrived made --jq print nothing; now it runs on what arrived and exits
0. After a signal, --jq stops at half of interruptGrace, so it flushes
what it printed before exitAfterGrace exits at the full grace; it ran
past that, and the forced exit dropped its buffered output. run() says
"Stopped before the response arrived" only for a request the signal
canceled, so a --jq that fails on fetched pages keeps its exit 2 error.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
92590fb restored the terminal when a signal stopped the hidden prompt,
but term.ReadPassword turns echo off inside the reading goroutine, so
a goroutine that got there after the restore left echo off: forced
with a barrier, 5 of 5 SIGTERM runs ended with -echo. Raw mode is now
set and undone by the caller, and the goroutine only reads, through
x/term's Terminal: 5 of 5 kept echo. Ctrl-C is a key in raw mode and
still exits 130 at once; Ctrl-D, which ReadPassword ignored, now gives
"No API key given", exit 2.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
README and --help say that login needs an absolute HOME (%AppData% on
Windows), as 571ac25 made it, and that zero_trace=true skips the cache
and leaves nothing for archive to read, as the large-result hint now
says (serpapi/SerpApi api_controller.rb and its zero_trace spec).

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
7f40182 gave --jq half of interruptGrace from when printing started,
so a slow decode after the signal could still push it past the forced
exit. Execute now cancels with a cause that records the signal time,
and both the watchdog and the print deadline count from it; --jq stops
a quarter of interruptGrace before the exit. The cause unwraps to
context.Canceled, since net/http reports a canceled request with the
context's cause: without that, a signal before the response turned
into network_error, exit 1.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
In raw mode (056ab9b) Ctrl-C reaches the hidden prompt as a key. Typed
in the same read as Enter, the line won: the key was saved and login
exited 0. And since the key canceled nothing, a stderr pipe that nobody
read hung the run with no watchdog. Now any Ctrl-C in the read stops it
with exit 130, nothing saved, and cancels the run as SIGINT does, so the
process exits within interruptGrace. PTY probe: Enter+^C in one write
130 at once (main: 130 after 3.1 s); ^C with stderr a full pipe exits at
3.1 s with echo on (before: still running at 8 s).

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
46af5c5's test let the watchdog's message goroutine outlive it, so
go test -race caught that goroutine reading output.IsTerminal while a
later test swapped it (2 of 4 runs). The test now waits for the
message. pkg/cmd under -race: 20 of 20 runs pass.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
With zero_trace=maybe and no_cache=true, the hint said the repeat "may"
be counted, though no_cache=true makes it counted under either Mongoid
reading of zero_trace. A flag that is surely on now wins over one that
is unsure.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A signal that lands after everything arrived and printed leaves exit 0:
the output is complete, and 130 would tell a script to retry billed
work. The README row and --help said 130 meant "stopped by Ctrl-C or
SIGTERM", which a review read as any signal; they now say it stopped
before it finished.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
v0.2.3 shipped on 2026-03-31; #7, #9, #15 and #17 merged after it and
none reached users, because a release waited for someone to push a tag.
Now, after both test legs and the live e2e pass on main, the tag job
tags the commit with the next version and calls release.yml: a patch,
or what the PR's release:minor or release:major label asks for. A
release:skip label, or a merge where nothing that ships changed since
the latest release, releases nothing. Outside pull requests a skipped
live test fails CI, a published tag is never reused, a commit older
than the latest release is never tagged, and runs on main go one at a
time. A Release preview check tells each PR what merging releases.
Dry run on this history: this branch with release:minor gives v0.3.0.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
release.yml uses .github/release-notes/<tag>.md as the release body when
it exists, so the v0.3.0 release gets these notes instead of GoReleaser's
list of commits.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
If the first automated release fails after its tag is made, re-running
the CI run's failed jobs repeats the same workflow. A workflow_dispatch
with a tag input runs release.yml from main for that tag once the cause
is fixed, without moving or re-pushing the tag.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The Release workflow now takes a tag from a dispatch input, which went
straight into actions/checkout's ref, a path and GoReleaser's arguments.
It must now look like vX.Y.Z, with an optional pre-release suffix: a
branch name, a path or "v0.3.0 --skip=validate" stops the run.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Only the merged pull request's label set the bump, so a release:major
merge whose run failed was released by the next, unlabeled merge as a
patch. Now a release:minor or release:major label on any pull request
merged since the latest release counts; release:skip still applies only
to the merge being released. On this history the lookup asks for 7
commits' pull requests, #2 to #15, none labeled.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
From the review of the auto-release: CI passes the tested SHA, and the
Release workflow stops if the tag points elsewhere, isn't a tag, or is
older than the newest release, which GoReleaser would mark latest and
Homebrew would roll back to. Runs on main and every publish wait their
turn (concurrency queue: max) instead of a newer one replacing a
waiting one. A re-run after a partial upload replaces the assets
(replace_existing_artifacts) instead of failing on them.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A docs-only pull request labeled release:major releases nothing, but its
label then turned the next unlabeled code merge into a major release. An
earlier pull request's release:minor or release:major label now counts
only if its own change touched what ships.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The preview looks up the labels of pull requests merged since the last
release through gh, which needs a token in Actions; without one it
failed whenever main had such merges, as it has now. It gets the run's
read-only token and pull-requests: read.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
bd29dc5's replace_existing_artifacts let a re-run replace a published
release's binaries, built with whatever Go is newest then, under the
same version. It is gone. Release now skips GoReleaser when the tag's
release has every file its checksums list, so a re-run only refreshes
Homebrew, and stops on a partial release until its assets are deleted.
On this repo's releases: v0.2.3 and v0.2.2 skip, v9.9.9 builds.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
From the advisor's review of the auto-release. Checkout takes
refs/tags/TAG, so a branch with the same name can't stand in for it,
and HEAD must equal the tag's commit. Building, uploading and the
Homebrew update share one job and one lock, so releases can't
interleave. Only a confirmed 404 means "no release yet": an API or
network error stops the run. A partial release is never rebuilt with a
newer Go; the next version fixes it forward. Checked against this
repo's releases: v0.2.3 and v0.2.2 skip, v9.9.9 builds, v0.1.1
(Rust-era assets) stops, an unreachable API stops.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The 27 cases that shaped release-tag.sh, from labels and the files that
ship to tags off main and API failures, lived outside the repo. They now
sit beside it and run in CI on every pull request and push; the tag job
waits for them, so a broken script can't tag.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@ilyazub ilyazub added the release:minor Merging releases the next minor version label Oct 13, 2026
@ilyazub
ilyazub merged commit 4aa0a9d into main Oct 13, 2026
8 of 9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

release:minor Merging releases the next minor version

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant