fix(cli): post-merge Bugbot fixes for market + files#2950
Merged
Conversation
Bugbot: append(path[:len(path)-1], "i18n") writes "i18n" into path[len(path)-1] whenever the variadic slice has spare capacity, silently mutating the caller's slice. fieldName is captured first so this function still resolves correctly, but other callers reading the slice afterwards see "i18n" in the last slot. Allocate a fresh prefix before appending so the caller's argument is never touched. Add TestResolveI18nFieldDoesNotMutateCaller covering both the happy path (i18n title resolved from the locale map) and a fixed-cap slice that would have been mutated by the prior code. Made-with: Cursor
Bugbot: runUpload accessed nodes[0].Name without a length check, panicking with index-out-of-range if the server returned an empty list. The lower-level client.FetchNodes already errors on empty data (internal/files/upload TestFetchNodes_EmptyList covers it), but a defensive guard at the call site keeps the failure mode at the user-visible layer instead of an opaque panic if a future regression weakens the lower layer. Made-with: Cursor
Bugbot: FrontendPath.HasTrailingSlash() returned the original input's trailing-slash flag, which disagreed with String() for the <fileType>/<extend> case (no subpath, no trailing slash). String() rendered "drive/Home/" because SubPath was synthesized to "/", but HasTrailingSlash() reported false — misleading callers branching on directory-vs-file. Drop the stored trailingSlash field and derive HasTrailingSlash() from strings.HasSuffix(SubPath, "/"), the same source of truth String() already uses. ParseFrontendPath still uses the local hadTrailingSlash to decide whether to re-append "/" to a deep sub-path; it just doesn't plumb it into the struct. Add a dedicated path_test case for "drive/Home" (no trailing slash) locking the previously-broken behavior down: HasTrailingSlash() now reports true, in agreement with String() == "drive/Home/". Made-with: Cursor
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 17a60b1. Configure here.
| // path[len(path)-1] whenever path has spare capacity. | ||
| i18nPath := make([]string, 0, len(path)) | ||
| i18nPath = append(i18nPath, path[:len(path)-1]...) | ||
| i18nPath = append(i18nPath, "i18n") |
There was a problem hiding this comment.
Slice-aliasing fix not applied to duplicate function
Medium Severity
The slice-aliasing fix in market/get.go's resolveI18nField was not applied to the identical copy in cli/cmd/ctl/app/get.go, which still uses the unsafe append(path[:len(path)-1], "i18n"). This is exactly the "inconsistent bug fix" risk that duplicated logic creates — the app package's resolveI18nField at line 155 retains the caller-mutation bug that this PR fixes in the market package.
Reviewed by Cursor Bugbot for commit 17a60b1. Configure here.
eball
approved these changes
Apr 26, 2026
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.
Summary
Three small, independently-reviewable Bugbot fixes that landed against #2949 after merge. Each one is a separate commit so reviewers can take them one at a time.
fix(cli): clone variadic path in market resolveI18nField—append(path[:len(path)-1], "i18n")mutated the caller's variadic slice whenever it had spare capacity. Allocate a fresh prefix and addTestResolveI18nFieldDoesNotMutateCaller. Addresses Bugbot3143232464.fix(cli): guard empty /api/nodes/ response in files upload—runUploadindexednodes[0]without a length check, panicking on an empty list.client.FetchNodesalready errors on empty data, but the call-site guard makes the failure mode explicit. Addresses Bugbot3143056106.fix(cli): derive HasTrailingSlash from SubPath for parse consistency—HasTrailingSlash()reported the raw-input flag, which disagreed withString()fordrive/Home(no trailing slash, no subpath). DeriveHasTrailingSlash()fromstrings.HasSuffix(SubPath, "/")so it always agrees withString(). Adds a dedicateddrive/Hometest case. Addresses Bugbot3143065025.The other four Bugbot reports from #2949 (
3142103059,3142103064,3142103065,3143042187) were already fixed in earlier follow-up commits inside #2949 itself; no additional work is needed for them in this PR..gitignorecleanups (/files/,/market/,cli/docs/) and the concurrentfmt.Fprintfincli/cmd/ctl/files/upload.goare intentionally out of scope (deferred per request / pre-existing low-severity).Test plan
cd cli && go build ./...cd cli && go vet ./cmd/ctl/market/... ./cmd/ctl/files/... ./internal/files/...cd cli && go test ./cmd/ctl/market/... ./cmd/ctl/files/... ./internal/files/...— all green, including the newTestResolveI18nFieldDoesNotMutateCallerand the newdrive/Homeno-trailing-slash case inpath_test.go.olares-cli market get firefoxstill resolves the i18n title (covered indirectly by the test fixture mirroring the production call shape).Made with Cursor
Note
Low Risk
Low risk: small defensive fixes in CLI-only helpers (path parsing, upload node selection, and market i18n field lookup) plus added regression tests; no protocol or data model changes.
Overview
Fixes a
filespath parsing inconsistency by derivingFrontendPath.HasTrailingSlash()fromSubPath, and treats bare<fileType>/<extend>(e.g.drive/Home) as the extend root soString()/directory semantics always agree; adds a regression test for the no-trailing-slash root case.Hardens
files uploadby explicitly erroring when/api/nodes/returns an empty list before indexingnodes[0], avoiding a potential panic.Prevents
market get’sresolveI18nFieldfrom mutating the caller’s variadicpathslice by allocating a new slice before appending, and adds a dedicated unit test to lock down the behavior.Reviewed by Cursor Bugbot for commit 17a60b1. Bugbot is set up for automated code reviews on this repo. Configure here.