Repository navigation
Conversation
491114e to
3081662
Compare
9132948 to
405e077
Compare
|
Commit: Regression test reportSelected 12 changed tests (limit=20, 0 not selected) 12 tests: regression=8 unreleased=0 coverage=4 failing=0
TestAccept/cmd/aitools/install-host-metadata: branch ✅ | main (8b4f052) ❌ | latest (v1.26.0) ❌main (8b4f052): latest (v1.26.0): TestAccept/cmd/aitools/telemetry/apps-init-tip: branch ✅ | main (8b4f052) ❌ | latest (v1.26.0) ❌main (8b4f052): latest (v1.26.0): TestAccept/cmd/aitools/telemetry/hint: branch ✅ | main (8b4f052) ❌ | latest (v1.26.0) ❌main (8b4f052): latest (v1.26.0): TestAccept/cmd/aitools/telemetry/install: branch ✅ | main (8b4f052) ❌ | latest (v1.26.0) ❌main (8b4f052): latest (v1.26.0): TestAccept/cmd/aitools/telemetry/install-failure: branch ✅ | main (8b4f052) ❌ | latest (v1.26.0) ❌main (8b4f052): latest (v1.26.0): TestAccept/cmd/aitools/telemetry/update-uninstall: branch ✅ | main (8b4f052) ❌ | latest (v1.26.0) ❌main (8b4f052): latest (v1.26.0): TestAccept/cmd/aitools/telemetry/workspace-unreachable: branch ✅ | main (8b4f052) ❌ | latest (v1.26.0) ❌main (8b4f052): latest (v1.26.0): TestAccept/cmd/version: branch ✅ | main (8b4f052) ❌ | latest (v1.26.0) ❌main (8b4f052): latest (v1.26.0): |
✅ Integration test reportCommit: 7f0dcdf
Top 12 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.
| // 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.
5275455 to
4982a91
Compare
|
Thanks. All in 4982a91. Question 1 ( The two inline comments are fixed and answered in their threads: transport failures while reading the body keep NETWORK/TIMEOUT, and the manifest read is capped at 1 MiB. |
f435418 to
7d445a5
Compare
4982a91 to
09de78d
Compare
3ab6a9a to
09533d4
Compare
09de78d to
c95fa79
Compare
09533d4 to
460a662
Compare
c95fa79 to
e0e7076
Compare
460a662 to
f1bf38a
Compare
e0e7076 to
af4dc8d
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 af4dc8d. Each finding was verified against the code; simonfaltum may follow up.
No blocking issues found, approving. 7 non-blocking comment(s) below.
This PR gives aitools install failures specific error categories through the typed FetchError and StateError errors and a TimedOut flag on BlockedError, while transport failures keep NETWORK/TIMEOUT. Every point from the earlier rounds is either fixed or answered, CI is green on every platform, and nothing blocking remains. What's left is small: a shared constant for the 1 MiB cap, a clearer error for an oversized manifest, a stale comment on the telemetry enum, a mid-body close that lands in a different category than a reset, a simplification in fetchErrorCategory, and changelog wording. Three already-answered threads are still open; you said #7480 handles them.
Comments outside the changed lines
- [nit]
.github/PULL_REQUEST_TEMPLATE.md:.agents/skills/pr-checklist/SKILL.mdasks agent-written PRs to follow.github/PULL_REQUEST_TEMPLATE.mdexactly (## Changes,## Why,## Tests, no new sections such as## Summary) and to keep the description short. This one uses## Summary,## Evidenceand## Merge Danger. Consider moving the content under the template headings and trimming the test logs.
Agreeing with existing threads
- [nice to have] A timed-out agent CLI still says cli-not-on-path: #7475 (comment)
- [nit] The timeout detail drops the agent CLI's stderr: #7475 (comment)
- [nit] StateError.Path and two FetchStage values are unused in production: #7475 (comment)
| } | ||
| // A response that arrived but names no usable release is still a failed | ||
| // release lookup, so these errors carry the stage too. | ||
| if err := json.NewDecoder(io.LimitReader(resp.Body, 1<<20)).Decode(&payload); err != nil { |
There was a problem hiding this comment.
[nit] source.go:22 adds maxManifestSize = 1 << 20, and its comment says the cap "matches the release lookup's", but this line still uses the literal io.LimitReader(resp.Body, 1<<20). If either value changes, the comment in source.go becomes wrong without anyone noticing. .agents/rules/style-guide-go.md asks to reuse an existing constant in the same package instead of adding a parallel literal. Use one shared constant (e.g. maxFetchBodySize) in both places.
There was a problem hiding this comment.
Done in 57c5e35. There is one shared maxFetchBodySize = 1 << 20 in errors.go, used by both the release lookup and FetchManifest. The separate maxManifestSize and the "matches the release lookup's" comment are gone.
| // The cap matches the release lookup's and is far above the manifest's size, | ||
| // so a large non-manifest body (a captive portal, a wrong mirror) is not read | ||
| // whole. | ||
| data, err := io.ReadAll(io.LimitReader(resp.Body, maxManifestSize)) |
There was a problem hiding this comment.
[nit] A manifest body over 1 MiB is cut off without notice. io.ReadAll(io.LimitReader(...)) returns the first 1 MiB with no error, and json.Unmarshal then fails with failed to parse manifest: unexpected end of JSON input (MANIFEST_INVALID). The real manifest is about 28 KB, so this only matters if it grows a lot. If it does, the message sends people looking for a broken file rather than a size limit. Read maxManifestSize+1 bytes and return an explicit "manifest is larger than 1 MiB" error when the extra byte arrives.
There was a problem hiding this comment.
Not changed. An explicit "larger than 1 MiB" error would no longer be a JSON decode error, so it would move an oversized body from MANIFEST_INVALID to SKILLS_FETCH_FAILED, which is the behavior this round keeps. Keeping that category would need another marker on FetchError. The real manifest is about 28 KB, so the cap only guards against a non-manifest body. If the manifest ever grows near the cap, that is the time to add a dedicated error.
| return stdlibErrorCategory(e.Err) | ||
| // The response arrived but reading its body failed on the transport (a | ||
| // timeout or a reset mid-body): still NETWORK or TIMEOUT. | ||
| case isTransportFailure(e.Err): |
There was a problem hiding this comment.
[nit] Body-read failures are still classified by error type. A connection that closes cleanly partway through the manifest or a skill file (io.ErrUnexpectedEOF from io.ReadAll, which is not a net.Error) reports SKILLS_FETCH_FAILED, in the same bucket as an HTTP 5xx. A reset at the same point reports NETWORK. Both are the transport failing after the response started; the connection closed mid-body case in TestClassifyRealManifestFetchErrors pins the current split. Now that FetchManifest reads and parses in separate steps, every failure of the read step is a transport failure. Consider flagging read-step failures on the FetchError and sending them through stdlibErrorCategory, treating io.ErrUnexpectedEOF as NETWORK. Otherwise, say in the changelog that a clean close mid-body reports SKILLS_FETCH_FAILED.
There was a problem hiding this comment.
Kept as is, so the categories don't change after approval. A clean close mid-body (io.ErrUnexpectedEOF) still reports SKILLS_FETCH_FAILED, and a reset reports NETWORK; TestClassifyRealManifestFetchErrors pins that split. I documented it in the PR body and the commit message ("a connection closed cleanly mid-body (SKILLS_FETCH_FAILED; a reset reports NETWORK)"). I left it out of the changelog fragment to keep it short, per the fragment comment.
| case e.StatusCode == 0: | ||
| return stdlibErrorCategory(e.Err) | ||
| // The response arrived but reading its body failed on the transport (a | ||
| // timeout or a reset mid-body): still NETWORK or TIMEOUT. | ||
| case isTransportFailure(e.Err): | ||
| return stdlibErrorCategory(e.Err) |
There was a problem hiding this comment.
[nit] Both transport cases return stdlibErrorCategory(e.Err), and isTransportFailure calls it once more, so a transport error is classified up to three times. Computing it once removes the duplicate case and the helper. After the context.Canceled check: if c := stdlibErrorCategory(e.Err); e.StatusCode == 0 || c == protos.AitoolsErrorCategoryNetwork || c == protos.AitoolsErrorCategoryTimeout { return c }.
There was a problem hiding this comment.
Done in 57c5e35. fetchErrorCategory now calls stdlibErrorCategory(e.Err) once, after the Canceled check, and returns it when StatusCode == 0 or when it is NETWORK/TIMEOUT. The duplicate case and isTransportFailure are gone. Behavior is unchanged and the existing tests pass as they are.
| // FetchError is checked before the standard library types: a response | ||
| // that arrived carries a status the net error types know nothing about. | ||
| if fetch, ok := errors.AsType[*installer.FetchError](err); ok { |
There was a problem hiding this comment.
[nit] The comment above these categories in libs/telemetry/protos/aitools_install.go:55-56 says "The typed installer errors (release lookup, skills fetch) win over the generic network/timeout classification." With this PR it is the other way round: a FetchError with a transport cause reports NETWORK or TIMEOUT, and the typed categories apply only once a response arrived. Someone reading the enum would expect every failed release lookup to be RELEASE_LOOKUP_FAILED. Update that comment to match.
There was a problem hiding this comment.
Updated in 57c5e35. The comment above the fetch categories in libs/telemetry/protos/aitools_install.go now says that a fetch failing on the transport (no response, or a timeout or reset while reading the body) reports NETWORK or TIMEOUT, and the other categories apply once a response arrived.
| @@ -0,0 +1 @@ | |||
| * `databricks aitools install --output json` now reports `SKILLS_FETCH_FAILED`, `RELEASE_LOOKUP_FAILED` or `RATE_LIMITED` as the `error_category` when a skills download or the latest-release lookup gets an error response (a fetch that fails on the transport, with no response or a timeout or reset while reading the body, still reports `NETWORK` or `TIMEOUT`), `MANIFEST_INVALID` for a skills manifest it cannot parse, `STATE_INVALID` for a corrupt install state file, and `PLUGIN_CLI_TIMEOUT` when a coding agent's CLI does not answer in time. ([#7475](https://github.com/databricks/cli/pull/7475)) | |||
There was a problem hiding this comment.
[nit] The fragment is a single sentence of about 80 words with a nested parenthetical, and much of it describes how the classification works (transport failure vs. error response) rather than what users see. It also leaves out a visible change: a timed-out agent CLI's message now reads timed out after <d> instead of the CLI's stderr. Consider listing the new error_category values briefly, with one clause about the timeout message.
There was a problem hiding this comment.
Shortened in 57c5e35. The fragment now lists the new error_category values, adds one sentence on the timed-out agent CLI message (timed out after <duration> instead of its stderr), and keeps the PR link.
af4dc8d to
57c5e35
Compare
|
Thanks for the approval. The nits are in 57c5e35: one shared 1 MiB constant, a single On the PR template: I kept the current body format so all PRs in this stack share one layout, which makes them easier to read side by side. The content maps onto the template: Summary is Changes and Why, Evidence is Tests, and Merge Danger covers the impact. I removed one stale line, the claim that the change was not run on Windows or macOS (CI runs both). |
f1bf38a to
21231db
Compare
57c5e35 to
8c24df0
Compare
21231db to
f6f7904
Compare
8c24df0 to
2d5e5ca
Compare
f6f7904 to
9f26165
Compare
2d5e5ca to
64a25aa
Compare
8f2ef4e to
ae31019
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) 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>
64a25aa to
7f0dcdf
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) 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>instead of whatever partial stderr the killed process left.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.