Skip to content

fix(FLEETMDM-003): 6 review findings across 3 files #128

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
flamingo wants to merge 3 commits into
base: main
Choose a base branch
from
+7 −8
Draft

fix(FLEETMDM-003): 6 review findings across 3 files #128

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b938f56
fix(FLEETMDM-003): 6 review findings across 3 files
flamingo[bot] Aug 24, 2026
cdd4d0d
fix(FLEETMDM-003): 6 review findings across 3 files
flamingo[bot] Aug 24, 2026
b60e5db
fix(FLEETMDM-003): 6 review findings across 3 files
flamingo[bot] Aug 24, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions orbit/pkg/go-paniclog/paniclog_unix.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
package paniclog

import (
"errors"
"fmt"
"os"

"golang.org/x/sys/unix"
Expand All @@ -16,20 +16,20 @@ func redirectStderr(f *os.File) (UndoFunction, error) {
stderrFd := int(os.Stderr.Fd())

@flamingo flamingo Bot Aug 24, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🦩 🟠 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

oldfd, err := unix.Dup(stderrFd)
if err != nil {
return nil, errors.New("Failed to redirect stderr to file: " + err.Error())
return nil, fmt.Errorf("redirect stderr to file: %w", err)
}

@flamingo flamingo Bot Aug 24, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🦩 🟠 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

err = unix.Dup2(int(f.Fd()), stderrFd)
if err != nil {
return nil, errors.New("Failed to redirect stderr to file: " + err.Error())
return nil, fmt.Errorf("redirect stderr to file: %w", err)
}

@flamingo flamingo Bot Aug 24, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🦩 🟠 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

undo := func() error {
undoErr := unix.Dup2(oldfd, stderrFd)
unix.Close(oldfd)

if undoErr != nil {
return errors.New("Failed to reverse stderr redirection: " + err.Error())
return fmt.Errorf("reverse stderr redirection: %w", undoErr)
}

return nil
Expand Down
4 changes: 2 additions & 2 deletions orbit/pkg/table/app_sso_platform/app_sso_platform_darwin.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -232,15 +232,15 @@ func parseAppSSOPlatformCommandOutput(output []byte, expectedExtensionIdentifier
}
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\"")
Comment on lines 232 to 246

@flamingo flamingo Bot Aug 24, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🦩 🟠 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

Comment on lines 232 to 246

@flamingo flamingo Bot Aug 24, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🦩 🟠 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

Expand Down
3 changes: 1 addition & 2 deletions orbit/pkg/table/cis_audit/cis_audit_windows.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -250,8 +250,7 @@ func getSeceditData() (SeceditData, error) {
// Load the .inf file content
cfg, err := ini.Load(fileContent)
if err != nil {
fmt.Printf("Error: %v\n", err)
return data, err
return data, fmt.Errorf("failed to load secedit inf file: %w", err)
}

@flamingo flamingo Bot Aug 24, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🦩 🟠 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


// Parse System Access section
Expand Down