fix(FLEETMDM-003): 6 review findings across 3 files - #128
Conversation
![flamingo[bot]](https://avatars.githubusercontent.com/in/3426709?s=60&v=4){kind=small}
| return nil, errors.New("Failed to redirect stderr to file: " + err.Error()) | ||
| return nil, fmt.Errorf("redirect stderr to file: %w", err) | ||
| } | ||
|
|
There was a problem hiding this comment.
🦩 🟠 Duplicate string-concatenation error swallows original error twice in undo path
In the undo closure inside redirectStderr, replaced errors.New("Failed to reverse stderr redirection: " + err.Error()) with fmt.Errorf("reverse stderr redirection: %w", undoErr), fixing the wrong-variable reference and adding proper %w wrapping.
🤖 Prompt for AI agents
In orbit/pkg/go-paniclog/paniclog_unix.go around line 26, review and complete this code-review fix: Duplicate string-concatenation error swallows original error twice in undo path.
What the draft fix changed: In the `undo` closure inside `redirectStderr`, replaced `errors.New("Failed to reverse stderr redirection: " + err.Error())` with `fmt.Errorf("reverse stderr redirection: %w", undoErr)`, fixing the wrong-variable reference and adding proper `%w` wrapping.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟢 90 high — react 👍/👎 to teach the reviewer
| @@ -16,20 +16,20 @@ func redirectStderr(f *os.File) (UndoFunction, error) { | |||
| stderrFd := int(os.Stderr.Fd()) | |||
There was a problem hiding this comment.
🦩 🟠 errors.New used instead of wrapped fmt.Errorf in orbit stderr redirection
In redirectStderr, replaced the first errors.New("Failed to redirect stderr to file: " + err.Error()) (after unix.Dup) with fmt.Errorf("redirect stderr to file: %w", err), preserving the error chain; also switched the import from errors to fmt since errors is no longer used.
🤖 Prompt for AI agents
In orbit/pkg/go-paniclog/paniclog_unix.go around line 16, review and complete this code-review fix: errors.New used instead of wrapped fmt.Errorf in orbit stderr redirection.
What the draft fix changed: In `redirectStderr`, replaced the first `errors.New("Failed to redirect stderr to file: " + err.Error())` (after `unix.Dup`) with `fmt.Errorf("redirect stderr to file: %w", err)`, preserving the error chain; also switched the import from `errors` to `fmt` since `errors` is no longer used.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟢 90 high — react 👍/👎 to teach the reviewer
| return nil, errors.New("Failed to redirect stderr to file: " + err.Error()) | ||
| return nil, fmt.Errorf("redirect stderr to file: %w", err) | ||
| } | ||
|
|
There was a problem hiding this comment.
🦩 🟠 Second Dup2 failure error also discards err chain
In redirectStderr, replaced the second errors.New("Failed to redirect stderr to file: " + err.Error()) (after unix.Dup2) with fmt.Errorf("redirect stderr to file: %w", err).
🤖 Prompt for AI agents
In orbit/pkg/go-paniclog/paniclog_unix.go around line 21, review and complete this code-review fix: Second Dup2 failure error also discards err chain.
What the draft fix changed: In `redirectStderr`, replaced the second `errors.New("Failed to redirect stderr to file: " + err.Error())` (after `unix.Dup2`) with `fmt.Errorf("redirect stderr to file: %w", err)`.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟢 90 high — react 👍/👎 to teach the reviewer
| } | ||
| realm, ok := realm_.(string) | ||
| if !ok { | ||
| return nil, fmt.Errorf("unexpected type for \"realm\" key in \"kerberosStatus\": %T", err) | ||
| return nil, fmt.Errorf("unexpected type for \"realm\" key in \"kerberosStatus\": %T", realm_) | ||
| } | ||
| upn_, ok := userConfig.KerberosStatus[0]["upn"] | ||
| if !ok { | ||
| return nil, errors.New("missing \"upn\" key in \"kerberosStatus\"") | ||
| } | ||
| upn, ok := upn_.(string) | ||
| if !ok { | ||
| return nil, fmt.Errorf("unexpected type for \"upn\" key in \"kerberosStatus\": %T", err) | ||
| return nil, fmt.Errorf("unexpected type for \"upn\" key in \"kerberosStatus\": %T", upn_) | ||
| } | ||
| if upn == "" { | ||
| return nil, errors.New("empty \"upn\" key in \"kerberosStatus\"") |
There was a problem hiding this comment.
🦩 🟠 Bare error return without wrapping in extractJSONSections/parseAppSSOPlatformCommandOutput helper chain
In parseAppSSOPlatformCommandOutput, the %T format argument for the "realm" type-assertion error was changed from err (always nil there) to realm_, so the error now correctly reports the actual unexpected type of the "realm" value.
🤖 Prompt for AI agents
In orbit/pkg/table/app_sso_platform/app_sso_platform_darwin.go around line 216, review and complete this code-review fix: Bare error return without wrapping in extractJSONSections/parseAppSSOPlatformCommandOutput helper chain.
What the draft fix changed: In parseAppSSOPlatformCommandOutput, the `%T` format argument for the "realm" type-assertion error was changed from `err` (always nil there) to `realm_`, so the error now correctly reports the actual unexpected type of the "realm" value.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟢 95 high — react 👍/👎 to teach the reviewer
| } | ||
| realm, ok := realm_.(string) | ||
| if !ok { | ||
| return nil, fmt.Errorf("unexpected type for \"realm\" key in \"kerberosStatus\": %T", err) | ||
| return nil, fmt.Errorf("unexpected type for \"realm\" key in \"kerberosStatus\": %T", realm_) | ||
| } | ||
| upn_, ok := userConfig.KerberosStatus[0]["upn"] | ||
| if !ok { | ||
| return nil, errors.New("missing \"upn\" key in \"kerberosStatus\"") | ||
| } | ||
| upn, ok := upn_.(string) | ||
| if !ok { | ||
| return nil, fmt.Errorf("unexpected type for \"upn\" key in \"kerberosStatus\": %T", err) | ||
| return nil, fmt.Errorf("unexpected type for \"upn\" key in \"kerberosStatus\": %T", upn_) | ||
| } | ||
| if upn == "" { | ||
| return nil, errors.New("empty \"upn\" key in \"kerberosStatus\"") |
There was a problem hiding this comment.
🦩 🟠 Same wrong-variable %T bug repeated for upn type assertion
In parseAppSSOPlatformCommandOutput, the %T format argument for the "upn" type-assertion error was changed from err (always nil there) to upn_, so the error now correctly reports the actual unexpected type of the "upn" value.
🤖 Prompt for AI agents
In orbit/pkg/table/app_sso_platform/app_sso_platform_darwin.go around line 224, review and complete this code-review fix: Same wrong-variable %T bug repeated for upn type assertion.
What the draft fix changed: In parseAppSSOPlatformCommandOutput, the `%T` format argument for the "upn" type-assertion error was changed from `err` (always nil there) to `upn_`, so the error now correctly reports the actual unexpected type of the "upn" value.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟢 95 high — react 👍/👎 to teach the reviewer
| fmt.Printf("Error: %v\n", err) | ||
| return data, err | ||
| return data, fmt.Errorf("failed to load secedit inf file: %w", err) | ||
| } |
There was a problem hiding this comment.
🦩 🟠 secedit.exe error printed with fmt.Printf and swallowed, not wrapped
In getSeceditData, replaced fmt.Printf("Error: %v\n", err); return data, err with return data, fmt.Errorf("failed to load secedit inf file: %w", err), removing the debug print and wrapping the ini.Load error with context and the %w verb to preserve the causal chain, matching the suggested fix exactly.
🤖 Prompt for AI agents
In orbit/pkg/table/cis_audit/cis_audit_windows.go around line 255, review and complete this code-review fix: secedit.exe error printed with fmt.Printf and swallowed, not wrapped.
What the draft fix changed: In `getSeceditData`, replaced `fmt.Printf("Error: %v\n", err); return data, err` with `return data, fmt.Errorf("failed to load secedit inf file: %w", err)`, removing the debug print and wrapping the `ini.Load` error with context and the `%w` verb to preserve the causal chain, matching the suggested fix exactly.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟢 92 high — react 👍/👎 to teach the reviewer
Closes 6 review findings across 3 files.
Draft — this is a starting point, not a finished change. The fix required judgment, so read it before trusting it.
orbit/pkg/go-paniclog/paniclog_unix.go:26orbit/pkg/go-paniclog/paniclog_unix.go:16orbit/pkg/go-paniclog/paniclog_unix.go:21orbit/pkg/table/app_sso_platform/app_sso_platform_darwin.go:216orbit/pkg/table/app_sso_platform/app_sso_platform_darwin.go:224orbit/pkg/table/cis_audit/cis_audit_windows.go:255What changed — and what was deliberately left — is explained per finding as inline review comments on the lines each finding touched.
Run: https://product-hub.flamingo.so/admin/code-review
Run id:
f6d23861-693f-45a1-b5b3-570aa3bc9ea7Merging this PR is recorded as acceptance of the rule that produced it;
closing it unmerged is recorded as rejection. Both feed rule health, so
closing a wrong suggestion is useful rather than merely tidy.