[NOT-861] feat(cli): use session-scoped file API - #70
Open
leo-notte wants to merge 1 commit into
Open
Conversation
|
| Filename | Overview |
|---|---|
| internal/cmd/files.go | Implements session-scoped file operations, but the default list request still filters to downloads and downloaded files lose expected permissions. |
| internal/cmd/files_test.go | Updates file-command tests for session endpoints, but does not cover the default-all query or permissions of the actual download command path. |
| internal/api/client.go | Removes the obsolete global uploaded-file download wrapper now that downloads use the session endpoint. |
| internal/api/client.gen.go | Removes obsolete file-storage fields from generated session request and response types. |
| internal/cmd/sessionstart_flags.gen.go | Removes generated registration and request mapping for the obsolete file-storage flag. |
| internal/cmd/sessionstart_optout.go | Removes the corresponding file-storage opt-out while retaining the other session-start opt-outs. |
Prompt To Fix All With AI
### Issue 1
internal/cmd/files.go:240-244
**Default listing filters downloads**
When `notte files list` is run without a source flag, the empty default-all value enters this `else` branch and adds `source=session_download`, causing user uploads to be omitted from a command documented to list all session files.
```suggestion
if source == filesSourceUploads {
endpoint += "&source=user_upload"
} else if source == filesSourceSession {
endpoint += "&source=session_download"
}
```
### Issue 2
internal/cmd/files.go:390-405
**Download replacement drops permissions**
Every successful download is renamed from an `os.CreateTemp` file without changing its 0600 mode, so new downloads are not ordinarily readable and overwriting an existing readable or executable file silently removes its previous permission bits.
```suggestion
fileMode := os.FileMode(0o644)
if info, statErr := os.Stat(destinationPath); statErr == nil {
if info.IsDir() {
return fmt.Errorf("destination path is a directory")
}
fileMode = info.Mode().Perm()
} else if !os.IsNotExist(statErr) {
return fmt.Errorf("failed to inspect destination: %w", statErr)
}
temporary, err := os.CreateTemp(filepath.Dir(destinationPath), ".notte-download-*")
if err != nil {
return err
}
temporaryPath := temporary.Name()
defer func() { _ = os.Remove(temporaryPath) }()
if _, err := io.Copy(temporary, resp.Body); err != nil {
_ = temporary.Close()
return err
}
if err := temporary.Chmod(fileMode); err != nil {
_ = temporary.Close()
return err
}
if err := temporary.Close(); err != nil {
return err
}
if err := os.Rename(temporaryPath, destinationPath); err != nil {
return err
}
```
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Reviews (1): Last reviewed commit: "feat(cli): use session-scoped file API" | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
240
to
244
| if source == filesSourceUploads { | ||
| ctx, cancel := GetContextWithTimeout(cmd.Context()) | ||
| defer cancel() | ||
|
|
||
| params := &api.FileListUploadsParams{} | ||
| resp, err := client.Client().FileListUploadsWithResponse(ctx, params) | ||
| if err != nil { | ||
| return fmt.Errorf("API request failed: %w", err) | ||
| } | ||
|
|
||
| if err := HandleAPIResponse(resp.HTTPResponse, resp.Body); err != nil { | ||
| return err | ||
| } | ||
|
|
||
| var fileNames []string | ||
| if resp.JSON200 != nil { | ||
| for _, f := range resp.JSON200.Files { | ||
| fileNames = append(fileNames, f.Name) | ||
| } | ||
| } | ||
| if printed, err := PrintListOrEmpty(fileNames, "No uploaded files."); err != nil { | ||
| return err | ||
| } else if printed { | ||
| return nil | ||
| } | ||
|
|
||
| if !IsJSONOutput() { | ||
| fmt.Println("Your uploaded files:") | ||
| } | ||
| return formatter.Print(fileNames) | ||
| endpoint += "&source=user_upload" | ||
| } else { | ||
| endpoint += "&source=session_download" | ||
| } |
There was a problem hiding this comment.
When notte files list is run without a source flag, the empty default-all value enters this else branch and adds source=session_download, causing user uploads to be omitted from a command documented to list all session files.
Suggested change
| if source == filesSourceUploads { | |
| ctx, cancel := GetContextWithTimeout(cmd.Context()) | |
| defer cancel() | |
| params := &api.FileListUploadsParams{} | |
| resp, err := client.Client().FileListUploadsWithResponse(ctx, params) | |
| if err != nil { | |
| return fmt.Errorf("API request failed: %w", err) | |
| } | |
| if err := HandleAPIResponse(resp.HTTPResponse, resp.Body); err != nil { | |
| return err | |
| } | |
| var fileNames []string | |
| if resp.JSON200 != nil { | |
| for _, f := range resp.JSON200.Files { | |
| fileNames = append(fileNames, f.Name) | |
| } | |
| } | |
| if printed, err := PrintListOrEmpty(fileNames, "No uploaded files."); err != nil { | |
| return err | |
| } else if printed { | |
| return nil | |
| } | |
| if !IsJSONOutput() { | |
| fmt.Println("Your uploaded files:") | |
| } | |
| return formatter.Print(fileNames) | |
| endpoint += "&source=user_upload" | |
| } else { | |
| endpoint += "&source=session_download" | |
| } | |
| if source == filesSourceUploads { | |
| endpoint += "&source=user_upload" | |
| } else if source == filesSourceSession { | |
| endpoint += "&source=session_download" | |
| } |
Prompt To Fix With AI
This is a comment left during a code review.
Path: internal/cmd/files.go
Line: 240-244
Comment:
**Default listing filters downloads**
When `notte files list` is run without a source flag, the empty default-all value enters this `else` branch and adds `source=session_download`, causing user uploads to be omitted from a command documented to list all session files.
```suggestion
if source == filesSourceUploads {
endpoint += "&source=user_upload"
} else if source == filesSourceSession {
endpoint += "&source=session_download"
}
```
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
Comment on lines
+390
to
405
| temporary, err := os.CreateTemp(filepath.Dir(destinationPath), ".notte-download-*") | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| if downloadResp.URL == "" { | ||
| return fmt.Errorf("no download URL in response") | ||
| temporaryPath := temporary.Name() | ||
| defer func() { _ = os.Remove(temporaryPath) }() | ||
| if _, err := io.Copy(temporary, resp.Body); err != nil { | ||
| _ = temporary.Close() | ||
| return err | ||
| } | ||
|
|
||
| // Determine output path | ||
| outputPath := filesDownloadOutput | ||
| if outputPath == "" { | ||
| outputPath = filename | ||
| if err := temporary.Close(); err != nil { | ||
| return err | ||
| } | ||
|
|
||
| if err := downloadFileWithContext(ctx, downloadResp.URL, outputPath); err != nil { | ||
| return fmt.Errorf("failed to download file: %w", err) | ||
| if err := os.Rename(temporaryPath, destinationPath); err != nil { | ||
| return err | ||
| } |
There was a problem hiding this comment.
Every successful download is renamed from an os.CreateTemp file without changing its 0600 mode, so new downloads are not ordinarily readable and overwriting an existing readable or executable file silently removes its previous permission bits.
Suggested change
| temporary, err := os.CreateTemp(filepath.Dir(destinationPath), ".notte-download-*") | |
| if err != nil { | |
| return err | |
| } | |
| if downloadResp.URL == "" { | |
| return fmt.Errorf("no download URL in response") | |
| temporaryPath := temporary.Name() | |
| defer func() { _ = os.Remove(temporaryPath) }() | |
| if _, err := io.Copy(temporary, resp.Body); err != nil { | |
| _ = temporary.Close() | |
| return err | |
| } | |
| // Determine output path | |
| outputPath := filesDownloadOutput | |
| if outputPath == "" { | |
| outputPath = filename | |
| if err := temporary.Close(); err != nil { | |
| return err | |
| } | |
| if err := downloadFileWithContext(ctx, downloadResp.URL, outputPath); err != nil { | |
| return fmt.Errorf("failed to download file: %w", err) | |
| if err := os.Rename(temporaryPath, destinationPath); err != nil { | |
| return err | |
| } | |
| fileMode := os.FileMode(0o644) | |
| if info, statErr := os.Stat(destinationPath); statErr == nil { | |
| if info.IsDir() { | |
| return fmt.Errorf("destination path is a directory") | |
| } | |
| fileMode = info.Mode().Perm() | |
| } else if !os.IsNotExist(statErr) { | |
| return fmt.Errorf("failed to inspect destination: %w", statErr) | |
| } | |
| temporary, err := os.CreateTemp(filepath.Dir(destinationPath), ".notte-download-*") | |
| if err != nil { | |
| return err | |
| } | |
| temporaryPath := temporary.Name() | |
| defer func() { _ = os.Remove(temporaryPath) }() | |
| if _, err := io.Copy(temporary, resp.Body); err != nil { | |
| _ = temporary.Close() | |
| return err | |
| } | |
| if err := temporary.Chmod(fileMode); err != nil { | |
| _ = temporary.Close() | |
| return err | |
| } | |
| if err := temporary.Close(); err != nil { | |
| return err | |
| } | |
| if err := os.Rename(temporaryPath, destinationPath); err != nil { | |
| return err | |
| } |
Knowledge Base Used: CLI Command Dispatch
Prompt To Fix With AI
This is a comment left during a code review.
Path: internal/cmd/files.go
Line: 390-405
Comment:
**Download replacement drops permissions**
Every successful download is renamed from an `os.CreateTemp` file without changing its 0600 mode, so new downloads are not ordinarily readable and overwriting an existing readable or executable file silently removes its previous permission bits.
```suggestion
fileMode := os.FileMode(0o644)
if info, statErr := os.Stat(destinationPath); statErr == nil {
if info.IsDir() {
return fmt.Errorf("destination path is a directory")
}
fileMode = info.Mode().Perm()
} else if !os.IsNotExist(statErr) {
return fmt.Errorf("failed to inspect destination: %w", statErr)
}
temporary, err := os.CreateTemp(filepath.Dir(destinationPath), ".notte-download-*")
if err != nil {
return err
}
temporaryPath := temporary.Name()
defer func() { _ = os.Remove(temporaryPath) }()
if _, err := io.Copy(temporary, resp.Body); err != nil {
_ = temporary.Close()
return err
}
if err := temporary.Chmod(fileMode); err != nil {
_ = temporary.Close()
return err
}
if err := temporary.Close(); err != nil {
return err
}
if err := os.Rename(temporaryPath, destinationPath); err != nil {
return err
}
```
**Knowledge Base Used:** [CLI Command Dispatch](https://app.greptile.com/nottelabs/-/custom-context/knowledge-base/nottelabs/notte-cli/-/docs/cli-commands.md)
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
giordano-lucas
changed the title
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
1 participant
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
Tests
go test ./internal/cmd ./internal/apiLinear: NOT-861 https://linear.app/nottelabsinc/issue/NOT-861/notte-cli-pr-70-featcli-use-session-scoped-file-api