Repository navigation
aitools: typed fetch, state and agent CLI timeout errors - #7475
Conversation
491114e to
3081662
Compare
9132948 to
405e077
Compare
|
Commit: Regression test reportSelected 1 changed tests (limit=20, 0 not selected) 1 tests: regression=1 unreleased=0 coverage=0 failing=0
TestAccept/cmd/aitools/telemetry/install-failure: branch ✅ | main (b48c9fb) ❌ | latest (v1.26.0) ❌main (b48c9fb): latest (v1.26.0): |
✅ Integration test reportCommit: 3f72232
Top 21 slowest tests (at least 2 minutes):
|
simonfaltum
left a comment
simonfaltum
left a comment
There was a problem hiding this comment.
🤖 Automated review from simonfaltum's review agent (pr-review skill) at 405e077. Each finding was verified against the code; simonfaltum may follow up.
Not approving yet: 1 open design question.
This PR adds typed installer errors and maps them to specific error_category values: SKILLS_FETCH_FAILED, RELEASE_LOOKUP_FAILED, RATE_LIMITED, MANIFEST_INVALID, STATE_INVALID and PLUGIN_CLI_TIMEOUT. It is well tested, lint-clean, and CI is green, and nothing found is blocking. The main points are these: transport failures (DNS errors, refused connections, timeouts) no longer report NETWORK/TIMEOUT in the released --output json contract, and the changelog doesn't mention it; and a timed-out agent CLI is still shown to the user as cli-not-on-path. The rest are small cleanups, plus one design question about reusing clicompat.HTTPStatusError.
Questions
FetchErroradds a typed HTTP-status error for the skills fetches, whileclicompat.HTTPStatusError(libs/clicompat/clicompat.go:28) already models a non-200 response from raw.githubusercontent.com, andclicompat.IsNotFoundErrorunderstands it. Was keeping two separate types intentional? If not, would you rather extend the clicompat type (stage, rate limit, wrapped cause) and reuse it here, or keepFetchErrorand makeIsNotFoundErrorrecognize its 404s, so that later status checks don't have to handle both?
Comments outside the changed lines
- [nit]
cmd/aitools/categorize_test.go:151: These cases buildfmt.Errorf("failed to fetch manifest: %w", <url.Error>)and expect NETWORK/TIMEOUT.FetchManifestnow returns that error inside aFetchError(source.go:37), so the real error classifies as SKILLS_FETCH_FAILED. The test passes while describing behavior production no longer has. Wrap the cases inFetchErrorand update the expected values, or drop the "failed to fetch manifest" prefix so they test the generic stdlib path. If transport errors go back to NETWORK/TIMEOUT (see the categorize.go comment), wrapping them inFetchError{StatusCode: 0}makes them valid regression tests for that. - [nit]
libs/aitools/installer/plugin.go:65: The doc comment says "Errors are *process.ProcessError", but on timeoutrunAgentCmdnow returns*agentCmdTimeoutError, which wraps one. A caller that trusts the contract and type-assertserr.(*process.ProcessError)gets a failed assertion on every timeout. Update it to say a timeout returns an error wrapping the ProcessError, so callers should useerrors.AsType.
| // The response arrived but was not a manifest: content, not transport. | ||
| case isManifestDecodeError(e): | ||
| return protos.AitoolsErrorCategoryManifestInvalid | ||
| default: |
There was a problem hiding this comment.
[nice to have] Every installer fetch now wraps its transport error in FetchError (source.go:37, installer.go:163, installer.go:255), and fetchErrorCategory has no transport case. A DNS failure, a refused connection or the 30s client timeout now all report SKILLS_FETCH_FAILED / RELEASE_LOOKUP_FAILED, the same as a GitHub 500 (funnel_test.go:334/374 show a refused connection moving from NETWORK to SKILLS_FETCH_FAILED). In both telemetry and the --output json error_category that shipped in v1.25.0, "the user can't reach GitHub" can no longer be told apart from "GitHub failed", and a script that retries on NETWORK/TIMEOUT silently stops matching. Merge Danger mentions NETWORK but not TIMEOUT, and lists the blast radius as telemetry-only. Either classify a FetchError with StatusCode == 0 through stdlibErrorCategory(e.Err) (after the Canceled check) to keep NETWORK/TIMEOUT, or say in the changelog fragment and Merge Danger that these failures no longer report NETWORK/TIMEOUT in --output json.
There was a problem hiding this comment.
Done in af32385: fetchErrorCategory now sends a FetchError with StatusCode == 0 (no response arrived) through stdlibErrorCategory, right after the Canceled check. So DNS failures and refused connections report NETWORK again, and client timeouts report TIMEOUT, the same as the released --output json contract. The specific categories now only apply to fetches that got a response. funnel_test.go is back to NETWORK for the refused connection. I added cases for a release lookup with no response (NETWORK), a manifest refused connection (NETWORK) and a skill file timeout (TIMEOUT). I also updated the changelog fragment and the Merge Danger section to match.
| // newBlockedError builds the BlockedError for a failed agent CLI command. | ||
| func newBlockedError(agent *agents.Agent, reason string, err error) *BlockedError { | ||
| _, timedOut := errors.AsType[*agentCmdTimeoutError](err) | ||
| return &BlockedError{Agent: agent.Name, Reason: reason, Detail: stderrOf(err), TimedOut: timedOut} |
There was a problem hiding this comment.
[nice to have] newBlockedError keeps the ReasonCLINotOnPath passed in by probePluginCLI, and BlockedError.Error() prints agent: reason: detail. A slow claude plugin --help therefore shows Skipped Claude Code: claude-code: cli-not-on-path: timed out after 20s, both in the text output (install.go:589) and in the JSON message (install.go:597). Only the telemetry category is fixed; the test comment says this used to be reported as "not on PATH", but the message still says it. A dedicated reason (e.g. ReasonCLITimedOut), set in newBlockedError when the timeout fired, would fix the message and the category together and remove the need for the TimedOut precedence rule in blockedErrorCategory.
There was a problem hiding this comment.
Not changed here. The later recovery-steps PR (#7480) branches on BlockedError.TimedOut and builds the user-facing line for a timeout itself ('claude plugin' timed out after 20s plus a re-run step), so the cli-not-on-path wording doesn't reach the text or JSON output once the stack lands. A new ReasonCLITimedOut would change the Reason contract that PR and #7482 depend on. I'd rather do it as a follow-up across the stack than in the middle of it.
There was a problem hiding this comment.
Our review found this too. One note on the reply: #7480 fixes the text output, but the JSON output still uses the full error. cmd/aitools/install.go:847 (in #7482) sets message: err.Error(), so --output json still says claude-code: cli-not-on-path: timed out after 20s. A coding agent that reads the JSON then gets the wrong cause.
There was a problem hiding this comment.
Right: #7480 changes only the text output. The JSON message is err.Error(), and BlockedError.Error() prints agent: reason: detail, so for a timeout it still shows cli-not-on-path. This PR leaves the JSON message as it was. The machine-readable signal for a timeout is error_category: PLUGIN_CLI_TIMEOUT, which this PR sets. The wording in Error() comes from Reason, so it gets fixed by the dedicated timeout reason discussed above. That would be a separate follow-up PR after the stack lands, because #7480 and #7482 depend on the current Reason values.
There was a problem hiding this comment.
Agreed, a follow-up after the stack is fine. I checked the base branch: a timeout already showed cli-not-on-path there, so this PR adds no regression. The JSON entry now also has error_category: PLUGIN_CLI_TIMEOUT, so an agent can find the correct cause. Please open an issue for ReasonCLITimedOut and link it here, so that the follow-up stays visible.
| // when no HTTP response arrived (DNS, proxy, TLS, timeout). | ||
| type FetchError struct { | ||
| Stage string | ||
| StatusCode int |
There was a problem hiding this comment.
[nit] In this PR nothing reads FetchError.StatusCode or StateError.Path, and Stage is only compared to FetchStageRelease. Yet the comments say stages exist "so the command layer can name the host the user has to reach" (lines 44-45) and that Path exists "so the user can be told which one to move aside" (line 93). Those uses arrive in #7480's recovery hints. That's fine if the stack lands together; if this PR could merge alone, reword the comments to describe what the fields do today, or move the fields into #7480.
There was a problem hiding this comment.
Reworded in af32385 so the comments describe what the fields do in this PR. Stages: "so a failed release lookup can be told apart from a failed download". StatusCode: it is now also read here (0 means no response, which maps to NETWORK/TIMEOUT). StateError.Path: "the state file that failed to parse".
| } | ||
|
|
||
| func (e *agentCmdTimeoutError) Error() string { | ||
| return fmt.Sprintf("timed out after %s", e.timeout) |
There was a problem hiding this comment.
[nit] agentCmdTimeoutError.Error() returns only timed out after <timeout>, and stderrOf now uses it instead of the process's stderr. With several plugin update or uninstall steps (pluginUpdateSteps, disable then uninstall), the message doesn't say which step hung. Include the subcommand in the message (e.g. plugin marketplace update timed out after 2m0s) by storing the argv, minus the binary path, on the error.
There was a problem hiding this comment.
Not changed. #7480 renders a timeout as '<binary> plugin' <Detail>, so putting the subcommand into Detail would print it twice ('claude plugin' plugin marketplace update timed out ...). Naming the step that hung fits better there, where the user-facing line is built. I'll pick it up in that PR.
| } | ||
| } | ||
|
|
||
| func fetchErrorCategory(e *installer.FetchError) protos.AitoolsErrorCategory { |
There was a problem hiding this comment.
[nit] .agents/rules/comments.md asks for a doc comment on Go functions, and this switch depends on its order: cancel, then rate limit, then not-found, then release stage, then decode. A one-line doc saying the first matching case wins, and why the release stage comes before the decode check, would stop a later reorder from silently changing categories.
There was a problem hiding this comment.
Added in af32385: a doc comment on fetchErrorCategory saying that the first matching case wins and explaining the order (interrupted, then no response, then the release stage before the decode check).
| return errors.New("signal: killed") | ||
| }) | ||
|
|
||
| _, err := runAgentCmd(ctx, time.Millisecond, []string{"claude", "plugin", "--help"}) |
There was a problem hiding this comment.
[nit] err from runAgentCmd goes straight into newBlockedError. If a regression ever made runAgentCmd return nil here, stderrOf(nil) would panic on err.Error() instead of failing the test with a clear message. Add require.Error(t, err) before building the BlockedError.
There was a problem hiding this comment.
Added require.Error(t, err) before building the BlockedError in af32385.
|
|
||
| func TestLoadStateCorruptReturnsStateError(t *testing.T) { | ||
| dir := t.TempDir() | ||
| require.NoError(t, os.WriteFile(filepath.Join(dir, ".state.json"), []byte("{not json"), 0o644)) |
There was a problem hiding this comment.
[nit] TestLoadStateCorruptReturnsStateError and TestLoadStateUnreadableIsNotStateError (lines 1596, 1601, 1609) use the literal ".state.json", but they are in package installer, which defines stateFileName (state.go:29), and state_test.go already uses it. Use the constant so a rename doesn't leave these tests checking the wrong file. These tests would also fit better in state_test.go, next to TestLoadStateCorruptJSON.
There was a problem hiding this comment.
Done in af32385: both tests moved to state_test.go, next to TestLoadStateCorruptJSON, and they now use stateFileName.
3081662 to
9075cd6
Compare
405e077 to
1aa2b9f
Compare
9075cd6 to
4b0c705
Compare
1aa2b9f to
bbd0a95
Compare
4b0c705 to
521c720
Compare
bbd0a95 to
f18c292
Compare
521c720 to
dd8dd43
Compare
f18c292 to
af32385
Compare
|
Thanks for the review. All in af32385: Question 1 (FetchError vs Outside-diff comments.
The inline threads have their own replies. I declined two, the timed-out reason and the step name in the timeout message, because the user-facing timeout line is built in #7480. |
dd8dd43 to
270ba1d
Compare
af32385 to
2408150
Compare
da3637c to
f435418
Compare
2408150 to
5275455
Compare
simonfaltum
left a comment
simonfaltum
left a comment
There was a problem hiding this comment.
🤖 Automated review from simonfaltum's review agent (pr-review skill) at 5275455. Each finding was verified against the code; simonfaltum may follow up.
Not approving yet: 1 open design question.
This PR gives aitools install failures specific error categories. It adds typed FetchError and StateError errors and a TimedOut flag on BlockedError, and most points from the earlier round are now addressed. One nice-to-have remains: a timeout or connection reset while the response body is being read now reports SKILLS_FETCH_FAILED or RELEASE_LOOKUP_FAILED instead of TIMEOUT or NETWORK. There is also a small nit about reading the manifest without a size cap, plus one question about the long-term plan for TimedOut vs a dedicated timeout reason. CI is green and nothing is blocking.
Questions
- A timeout is now carried as a
TimedOutflag next toReason. A slowclaude plugin --helptherefore becomesReason=cli-not-on-path, TimedOut=true:Error()still printscli-not-on-path, and every consumer has to check the flag before switching onReason(blockedErrorCategoryhere, the recovery step in #7480). You mentioned moving to a dedicated timeout reason in a follow-up across the stack, since #7480 and #7482 depend on the currentReasonvalues. Is that follow-up tracked somewhere, such as an issue or a specific later PR in this stack? Or do you plan to keep the flag as the long-term contract? If you keep it, could theBlockedErrordoc say thatTimedOuttakes precedence overReason?
Agreeing with existing threads
- [nit] A timed-out probe still says cli-not-on-path: #7475 (comment)
- [nit] The timeout detail drops the command and its stderr: #7475 (comment)
| // No response arrived (DNS, refused connection, proxy, TLS, client | ||
| // timeout): the user cannot reach the host, which NETWORK and TIMEOUT | ||
| // already report. | ||
| case e.StatusCode == 0: |
There was a problem hiding this comment.
[nice to have] A timeout or connection reset while the response body is being read now loses its TIMEOUT/NETWORK category. Those FetchErrors already carry StatusCode: 200, so they skip the StatusCode == 0 case. A manifest or skill file download that hits the 30s client timeout mid-body reports SKILLS_FETCH_FAILED, a release lookup that does reports RELEASE_LOOKUP_FAILED, and a reset mid-body reports SKILLS_FETCH_FAILED. At the base commit these were TIMEOUT and NETWORK in --output json.
I reproduced it with a test server that sends the headers and part of the manifest and then stalls: TIMEOUT before, SKILLS_FETCH_FAILED now. A mid-body RST gives NETWORK before, SKILLS_FETCH_FAILED now. The changelog and Merge Danger only mention fetches that got an error response.
Suggest routing any FetchError whose cause stdlibErrorCategory reads as NETWORK or TIMEOUT through that path, e.g. case e.StatusCode == 0 || isTransportFailure(e.Err): return stdlibErrorCategory(e.Err). A clean early close (io.ErrUnexpectedEOF) still reports SKILLS_FETCH_FAILED, so the existing TestClassifyRealManifestFetchErrors cases still pass. A stalled-body case would cover the new path.
There was a problem hiding this comment.
Fixed in 4982a91. fetchErrorCategory now also routes a FetchError whose cause stdlibErrorCategory reads as NETWORK or TIMEOUT through that path (isTransportFailure), after the StatusCode == 0 case. A stalled body or a reset mid-body now reports TIMEOUT or NETWORK again, for the manifest, a skill file and the release lookup. A clean early close (io.ErrUnexpectedEOF) still reports SKILLS_FETCH_FAILED, and the existing TestClassifyRealManifestFetchErrors cases are unchanged. The new TestClassifyManifestBodyTransportFailure covers a real stalled body against a test server (TIMEOUT), a reset mid-body (NETWORK) and a release-body timeout (TIMEOUT). It fails without the fix. I updated the changelog fragment and Merge Danger to say transport failures keep NETWORK/TIMEOUT.
| // Read the whole body before parsing it: a connection dropped mid-body is a | ||
| // fetch failure, while a body that arrived but is empty or truncated JSON is | ||
| // an invalid manifest. A streaming decoder reports both as io.ErrUnexpectedEOF. | ||
| data, err := io.ReadAll(resp.Body) |
There was a problem hiding this comment.
[nit] io.ReadAll(resp.Body) has no size cap, while the release lookup reads through io.LimitReader(resp.Body, 1<<20). With the old streaming decoder, a 200 response that isn't JSON (a captive portal, or a misconfigured DATABRICKS_SKILLS_BASE_URL mirror) failed at its first byte. Now the whole body is read into memory, for up to the 30s client timeout, before it is reported as MANIFEST_INVALID. Wrap the body in an io.LimitReader with a cap well above the real manifest's size.
There was a problem hiding this comment.
Fixed in 4982a91: the manifest body is now read through io.LimitReader(resp.Body, maxManifestSize) with maxManifestSize = 1 << 20, the same cap as the release lookup. The real manifest is far below that. A body over the cap is cut off and reports MANIFEST_INVALID. There is a new case for it in TestClassifyRealManifestFetchErrors.
57c5e35 to
8c24df0
Compare
21231db to
f6f7904
Compare
8c24df0 to
2d5e5ca
Compare
f6f7904 to
9f26165
Compare
2d5e5ca to
64a25aa
Compare
8f2ef4e to
ae31019
Compare
64a25aa to
7f0dcdf
Compare
| // to the error's own message. Callers must not branch on this string. | ||
| func stderrOf(err error) string { | ||
| // A killed process has partial or no stderr; the timeout is the real cause. | ||
| if terr, ok := errors.AsType[*agentCmdTimeoutError](err); ok { |
There was a problem hiding this comment.
[nit] On a timeout, stderrOf returns only "timed out after X" and drops the stderr that the CLI wrote before it was killed. That text often tells why the CLI hung, for example a login prompt that waits for input. Keep both: "timed out after 20s: ".
There was a problem hiding this comment.
Done in 4deb804. On a timeout, stderrOf now returns timed out after <timeout> followed by whatever the CLI wrote to stderr before it was killed, e.g. timed out after 20s: Waiting for login.... With no stderr it is still timed out after <timeout>. The new TestRunAgentCmdTimeoutKeepsPartialStderr covers it. TimedOut and the category are unchanged.
There was a problem hiding this comment.
Verified at 43ac795. On a timeout, stderrOf now returns timed out after <timeout>: <stderr>. With no stderr, it returns only timed out after <timeout>. TestRunAgentCmdTimeoutKeepsPartialStderr passes, and it fails when I remove the stderr suffix. Note: 4deb804 is not on the branch now. 43ac795 has the same patch on the new base. Thank you.
ae31019 to
bb8734f
Compare
7f0dcdf to
45cb4be
Compare
bb8734f to
7a75603
Compare
45cb4be to
dccb816
Compare
7a75603 to
9138ddc
Compare
dccb816 to
4deb804
Compare
9138ddc to
ae5eede
Compare
4deb804 to
43ac795
Compare
rugpanov
left a comment
rugpanov
left a comment
There was a problem hiding this comment.
I checked the three replies against 43ac795. The two fixes work as the replies say, and the new tests catch a revert of each fix. go test passes for libs/aitools/installer, cmd/aitools, libs/process and libs/telemetry/protos, and for the cmd/aitools acceptance tests (Linux). The JSON wording can wait for the follow-up after the stack.
ae5eede to
276f78c
Compare
43ac795 to
9ef9292
Compare
276f78c to
e84d3cc
Compare
9ef9292 to
6bd9077
Compare
e84d3cc to
9bfea48
Compare
`aitools install` failures that used to be `UNCATEGORIZED` now get a specific `error_category`. The installer returns typed errors and `classifyInstallError` maps them:
```text
classifyInstallError(err)
BlockedError
TimedOut -> PLUGIN_CLI_TIMEOUT (agent CLI killed by its timeout)
FetchError{Stage, StatusCode, RateLimited} (release / manifest / skill file)
context.Canceled -> COMMAND_INTERRUPTED
transport failure -> NETWORK / TIMEOUT (no response, or a timeout/reset while reading the body)
RateLimited -> RATE_LIMITED (429, or 403 with GitHub's rate-limit headers)
clicompat.ErrNotFound -> MANIFEST_NOT_FOUND
Stage == release -> RELEASE_LOOKUP_FAILED
JSON syntax/type error -> MANIFEST_INVALID
otherwise -> SKILLS_FETCH_FAILED
StateError{Path} -> STATE_INVALID (unparsable .state.json)
```
- `FetchError` and `StateError` keep the wrapped error's message, so what the user sees does not change.
- A fetch that fails on the transport (no response, or a timeout or reset while reading the body) keeps reporting `NETWORK` or `TIMEOUT`, as before.
- A missing manifest wraps `clicompat.ErrNotFound`, so `clicompat.IsNotFoundError` stays the one way to check for a missing ref.
- An unreadable state file is not a `StateError`: it keeps its filesystem category.
- `FetchManifest` reads the whole body (capped at 1 MiB, like the release lookup; a larger body is reported as too large, `MANIFEST_INVALID`) before parsing it, so a connection closed cleanly mid-body (`SKILLS_FETCH_FAILED`; a reset reports `NETWORK`) is told apart from an empty or truncated manifest (`MANIFEST_INVALID`).
- Changelog fragment: `.nextchanges/cli/aitools-install-fetch-error-categories.md`.
Co-authored-by: Isaac <no-reply@databricks.com>
6bd9077 to
3f72232
Compare
Stacked on #7474.
Stacked on: aitools: do not block plugin installs on the latest-release lookup
Summary
aitools installfailures that used to beUNCATEGORIZEDnow get a specificerror_category. The installer returns typed errors andclassifyInstallErrormaps them:FetchErrorandStateErrorkeep the wrapped error's message, so what the user sees does not change.NETWORKorTIMEOUT, as before.clicompat.ErrNotFound, soclicompat.IsNotFoundErrorstays the one way to check for a missing ref.StateError: it keeps its filesystem category.FetchManifestreads the whole body (capped at 1 MiB, like the release lookup; a larger body is reported as too large,MANIFEST_INVALID) before parsing it, so a connection closed cleanly mid-body (SKILLS_FETCH_FAILED; a reset reportsNETWORK) is told apart from an empty or truncated manifest (MANIFEST_INVALID)..nextchanges/cli/aitools-install-fetch-error-categories.md.Evidence
Run at this head, with
libs/aitools/installer/source.goandstate.gotemporarily set back to the parent's version (not committed):Merge Danger
Door: two-way
Reclassification of
error_category(telemetry and--output json) plus typed wrappers; reverting restores the old categories.Blast Radius: telemetry and the
error_categoryfield ofaitools install --output jsonUNCATEGORIZEDto the categories above; fetches that fail on the transport (no response, or a timeout or reset mid-body) still reportNETWORKorTIMEOUT.timed out after <timeout>, followed by whatever stderr it wrote before it was killed.FetchManifestnow rejects trailing data after the manifest object (json.Unmarshalinstead of a streaming decoder).TestLoadStateUnreadableIsNotStateErrorskips on Windows and as root.This pull request and its description were written by Isaac.