feature/code cleanup app #111
Conversation
Summary by CodeRabbitRelease Notes
WalkthroughThis PR refactors the TUI application architecture by extracting types and message handlers from a monolithic Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant KeyboardRouter as Keyboard Router<br/>(handleKeyMsg)
participant ScreenHandler as Screen Handler<br/>(e.g., handleEvalFormInput)
participant StateManager as Model State
participant Command as Command/Msg
participant Sidebar as Sidebar/Dialog
User->>KeyboardRouter: Key Event
rect rgb(220, 240, 255)
note over KeyboardRouter: Check precedence
KeyboardRouter->>Sidebar: LLM Dialog open?
Sidebar-->>KeyboardRouter: Handle if open
end
rect rgb(240, 240, 240)
note over KeyboardRouter: Route to screen
KeyboardRouter->>ScreenHandler: Route by activeScreen
ScreenHandler->>StateManager: Update state<br/>(e.g., cursor, field focus)
ScreenHandler->>Command: Generate command<br/>(e.g., healthCheckCmd)
end
Command-->>KeyboardRouter: Return updated Model + Cmd
KeyboardRouter-->>User: Render updated View
sequenceDiagram
participant Model
participant CommonController as Message Handlers<br/>(common_controller.go)
participant SubComponent as Sub-Component<br/>(e.g., ScenarioEditor)
participant StateManager as Internal State
participant Render as View Renderer
Model->>CommonController: handlePasteMsg / handleSpinnerTickMsg / etc.
rect rgb(240, 255, 240)
note over CommonController: Route & Update
CommonController->>SubComponent: Forward or delegate<br/>(e.g., Paste to ScenarioEditor)
SubComponent->>StateManager: Update sub-state
end
rect rgb(255, 240, 240)
note over StateManager: Coordinate
StateManager->>StateManager: Update focused field,<br/>viewport, spinner state
end
CommonController-->>Model: Return (Model, tea.Cmd)
Model->>Render: Call View()
Render-->>Model: Render TUI
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes The refactoring follows a consistent pattern across multiple files (extracting handlers into dedicated controller methods), reducing individual file complexity. However, the breadth of changes (11 new files, significant reorganization of state/message flow), keyboard routing logic density, and screen-specific handler implementations require careful review to verify control flow correctness, state consistency, and proper delegation across the modular structure. Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/tui/internal/tui/app.go (2)
15-20: Fix fallback theme: avoid setting an unregistered theme.On JSON load failure you register "default" but set "aura". This can leave CurrentTheme() nil and cause panics later.
if err := theme.LoadThemesFromJSON(); err != nil { fmt.Printf("Warning: Failed to load themes: %v\n", err) // Create a fallback theme if loading fails theme.RegisterTheme("default", theme.NewSystemTheme(nil, true)) - theme.SetTheme("aura") + theme.SetTheme("default") }
59-60: Guard invalid theme from config.If the configured theme isn’t registered, fall back to "default" to keep UI stable.
theme.SetTheme(model.config.Theme) + if theme.CurrentTheme() == nil { + fmt.Printf("Warning: Unknown theme %q, falling back to default\n", model.config.Theme) + theme.SetTheme("default") + }
🧹 Nitpick comments (15)
packages/tui/internal/tui/app_types.go (1)
61-63: Avoid copying large dialogs; store pointers in the stack.Dialog contains funcs/embedded state; pushing copies risks subtle bugs and higher allocations. Prefer []*components.Dialog for dialogStack.
- dialogStack []components.Dialog + dialogStack []*components.Dialogpackages/tui/internal/tui/help_controller.go (1)
11-30: Use typed key codes; simplify command handling.Comparing msg.String() to "home"/"end" can vary by terminal/layout. Prefer msg.Type == tea.KeyHome/tea.KeyEnd; also no need to batch a single cmd.
- switch msg.String() { - case "home": + switch msg.Type { + case tea.KeyHome: m.helpViewport.GotoTop() return m, nil - - case "end": + case tea.KeyEnd: m.helpViewport.GotoBottom() return m, nil - - default: - // Update the help viewport for scrolling - helpViewportPtr, cmd := m.helpViewport.Update(msg) - if cmd != nil { - cmds = append(cmds, cmd) - } - m.helpViewport = *helpViewportPtr - return m, tea.Batch(cmds...) + default: + vp, cmd := m.helpViewport.Update(msg) + m.helpViewport = *vp + return m, cmd }Also consider using tea.WithUniformKeyLayout() when constructing the program to normalize key events across platforms. Based on learnings
packages/tui/internal/tui/report_controller.go (2)
47-65: Avoid double-processing scroll keys; rely on a single path.You both call ScrollUp/Down and then forward the same key to reportHistory.Update(msg). That likely applies the scroll twice and can cause jitter. Use one approach (prefer Update(msg)).
Apply this to streamline:
case "up", "down", "pgup", "pgdown": // Scroll the report if m.reportHistory != nil { - switch msg.String() { - case "up": - m.reportHistory.ScrollUp(1) - case "down": - m.reportHistory.ScrollDown(1) - case "pgup": - m.reportHistory.ScrollUp(10) - case "pgdown": - m.reportHistory.ScrollDown(10) - } - cmd := m.reportHistory.Update(msg) + cmd := m.reportHistory.Update(msg) if cmd != nil { cmds = append(cmds, cmd) } } return m, tea.Batch(cmds...)
28-29: Redundant spinner activation.SetActive(true) is unnecessary if you immediately return summarySpinner.Start(). Start() should manage activation.
- m.summarySpinner.SetActive(true) - return m, tea.Batch(m.summarySpinner.Start(), m.summaryGenerationCmd()) + return m, tea.Batch(m.summarySpinner.Start(), m.summaryGenerationCmd())packages/tui/internal/tui/eval_detail_controller.go (1)
100-115: Deduplicate scrolling to prevent double moves and jitter.You perform manual ScrollUp/Down and then call Update(msg), which likely scrolls again. Keep a single source of truth (prefer Update(msg)), preserving your boundary-switch logic.
Example for eventsHistory block:
- m.eventsHistory.Focus() - switch msg.String() { - case "up": - m.eventsHistory.ScrollUp(1) - case "down": - m.eventsHistory.ScrollDown(1) - case "pgup": - m.eventsHistory.ScrollUp(10) - case "pgdown": - m.eventsHistory.ScrollDown(10) - } - cmd := m.eventsHistory.Update(msg) + m.eventsHistory.Focus() + cmd := m.eventsHistory.Update(msg)Apply the same simplification to summaryHistory.
Also applies to: 126-141
packages/tui/internal/tui/app_commands.go (1)
127-141: clampToInt: OK but strconv.Atoi is simpler.Current code is fine. Consider strconv.Atoi for clarity.
packages/tui/internal/tui/keyboard_controller.go (1)
311-329: Missing InterviewScreen routing.Ctrl+I sets InterviewScreen, but routeKeyToScreen has no case for it. Keys will be ignored on that screen.
case ReportScreen: return m.handleReportInput(msg) +case InterviewScreen: + return m.handleInterviewInput(msg) // or no-op handler to keep UX consistentIf there’s no handler yet, add a stub that returns m, nil.
packages/tui/internal/tui/config_controller.go (3)
31-39: Verify dialog semantics for single “Save” button and action wiring.You replace default OK/Cancel with a single “Save” button (Action: save_qualifire). Ensure components.Dialog.Update handles one-button input dialogs and that there’s logic consuming "save_qualifire" to persist the key.
I can add the handler to common_controller.go if missing.
Also applies to: 145-166
217-221: Handle theme.SetTheme errors and persist only on success.SetTheme can fail (unknown theme). Capture the error; avoid saving an invalid theme.
- if selectedTheme != theme.CurrentThemeName() { - m.config.Theme = selectedTheme - theme.SetTheme(selectedTheme) - m.configState.HasChanges = true - } + if selectedTheme != theme.CurrentThemeName() { + if err := theme.SetTheme(selectedTheme); err == nil { + m.config.Theme = selectedTheme + m.configState.HasChanges = true + } else { + // optionally surface a toast/dialog; at least don’t persist a bad value + } + }
206-210: Server URL save only when active field matches.This saves URL only if ActiveField == ServerURL. If users navigate away before hitting Enter, changes might be lost. Consider saving when leaving the field as you already partly do in “down”.
packages/tui/internal/tui/app_interview.go (1)
124-148: Unify interview model fallback and key sourcing (consistency).Defaults/fallbacks differ across flows; consider a single helper to derive provider/model/key from config with clear precedence to avoid drift.
packages/tui/internal/tui/app.go (1)
64-65: Opt into Bubble Tea v2 keyboard improvements (optional).Uniform key layout and key releases improve cross‑platform UX.
- p := tea.NewProgram(model, tea.WithAltScreen(), tea.WithMouseCellMotion()) + p := tea.NewProgram( + model, + tea.WithAltScreen(), + tea.WithMouseCellMotion(), + tea.WithUniformKeyLayout(), // v2 + // tea.WithKeyReleases(), // enable if you handle key-up events + )Based on learnings.
packages/tui/internal/tui/common_controller.go (3)
27-35: Sanitize CRLF on paste (Windows).Remove “\r” too to avoid stray characters in inputs.
- cleanText := strings.TrimSpace(strings.ReplaceAll(string(msg), "\n", "")) + cleanText := strings.TrimSpace(strings.NewReplacer("\r", "", "\n", "").Replace(string(msg))) @@ - cleanText := strings.TrimSpace(strings.ReplaceAll(string(msg), "\n", "")) + cleanText := strings.TrimSpace(strings.NewReplacer("\r", "", "\n", "").Replace(string(msg)))Also applies to: 41-45
82-89: Avoid batching nil commands (minor hygiene).Filter nil before append to keep tea.Batch clean.
- m.healthSpinner, cmd = m.healthSpinner.Update(msg) - cmds = append(cmds, cmd) + m.healthSpinner, cmd = m.healthSpinner.Update(msg) + if cmd != nil { + cmds = append(cmds, cmd) + } - m.summarySpinner, cmd = m.summarySpinner.Update(msg) - cmds = append(cmds, cmd) + m.summarySpinner, cmd = m.summarySpinner.Update(msg) + if cmd != nil { + cmds = append(cmds, cmd) + } - m.evalSpinner, cmd = m.evalSpinner.Update(msg) - cmds = append(cmds, cmd) + m.evalSpinner, cmd = m.evalSpinner.Update(msg) + if cmd != nil { + cmds = append(cmds, cmd) + }
103-122: Refresh markdown renderer on resize (keeps wrapping correct).Renderer cache depends on width; update the editor’s renderer after size changes.
m.helpViewport.SetSize(viewportWidth, viewportHeight) - return m, nil + // Refresh markdown renderer for the new width when relevant + if m.currentScreen == ScenariosScreen { + m.scenarioEditor.SetMarkdownRenderer(m.getMarkdownRenderer()) + } + return m, nil
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
packages/tui/internal/tui/app.go(1 hunks)packages/tui/internal/tui/app_commands.go(1 hunks)packages/tui/internal/tui/app_config.go(1 hunks)packages/tui/internal/tui/app_interview.go(1 hunks)packages/tui/internal/tui/app_types.go(1 hunks)packages/tui/internal/tui/common_controller.go(1 hunks)packages/tui/internal/tui/config_controller.go(1 hunks)packages/tui/internal/tui/dashboard_controller.go(1 hunks)packages/tui/internal/tui/eval_detail_controller.go(1 hunks)packages/tui/internal/tui/eval_form_controller.go(1 hunks)packages/tui/internal/tui/help_controller.go(1 hunks)packages/tui/internal/tui/keyboard_controller.go(1 hunks)packages/tui/internal/tui/report_controller.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (12)
packages/tui/internal/tui/eval_detail_controller.go (2)
packages/tui/internal/tui/app_types.go (3)
Model(49-88)DashboardScreen(32-32)ReportScreen(36-36)packages/tui/internal/components/dialog.go (1)
NewReportPersistenceDialog(130-145)
packages/tui/internal/tui/config_controller.go (4)
packages/tui/internal/tui/app_types.go (1)
Model(49-88)packages/tui/internal/tui/configurations.go (3)
ConfigFieldQualifire(16-16)ConfigFieldServerURL(14-14)ConfigFieldTheme(15-15)packages/tui/internal/components/dialog.go (3)
NewInputDialog(98-113)DialogButton(22-26)PrimaryButton(32-32)packages/tui/internal/theme/manager.go (3)
AvailableThemes(87-109)CurrentThemeName(79-84)SetTheme(49-63)
packages/tui/internal/tui/app_config.go (1)
packages/tui/internal/tui/app_types.go (1)
Model(49-88)
packages/tui/internal/tui/keyboard_controller.go (5)
packages/tui/internal/tui/app_types.go (10)
Model(49-88)HelpScreen(40-40)InterviewScreen(37-37)NewEvaluationScreen(35-35)ScenariosScreen(39-39)ConfigurationScreen(38-38)ConfigState(119-127)DashboardScreen(32-32)ReportScreen(36-36)EvaluationDetailScreen(34-34)packages/tui/internal/tui/eval_ui.go (1)
EvaluationViewState(11-36)packages/tui/internal/components/llm_config_dialog.go (1)
NewLLMConfigDialog(183-270)packages/tui/internal/tui/configurations.go (1)
ConfigFieldServerURL(14-14)packages/tui/internal/components/dialog.go (1)
NewReportPersistenceDialog(130-145)
packages/tui/internal/tui/app_commands.go (2)
packages/tui/internal/tui/app_types.go (6)
AutoRefreshMsg(11-11)Model(49-88)HealthCheckResultMsg(14-17)StartEvaluationMsg(20-20)SummaryGeneratedMsg(23-26)Scenario(98-102)packages/tui/internal/tui/evaluation.go (1)
NewRogueSDK(112-119)
packages/tui/internal/tui/app_types.go (9)
packages/tui/internal/components/command_line.go (1)
CommandInput(21-30)packages/tui/internal/components/dialog.go (1)
Dialog(38-51)packages/tui/internal/components/llm_config_dialog.go (1)
LLMConfigDialog(130-150)packages/tui/internal/components/scenario_editor.go (1)
ScenarioEditor(12-67)packages/tui/internal/components/spinner.go (1)
Spinner(16-22)packages/tui/internal/components/message_history_view.go (1)
MessageHistoryView(20-45)packages/tui/internal/components/viewport.go (1)
Viewport(52-71)packages/tui/internal/tui/eval_ui.go (1)
EvaluationViewState(11-36)packages/tui/internal/tui/configurations.go (1)
ConfigField(11-11)
packages/tui/internal/tui/app_interview.go (5)
packages/tui/internal/tui/app_types.go (2)
Model(49-88)Scenario(98-102)packages/tui/internal/tui/evaluation.go (1)
NewRogueSDK(112-119)packages/tui/internal/components/scenario_types.go (3)
InterviewStartedMsg(44-48)InterviewResponseMsg(50-55)ScenariosGeneratedMsg(57-61)packages/tui/internal/theme/manager.go (2)
CurrentTheme(67-76)CurrentThemeName(79-84)packages/tui/internal/tui/markdown_renderer.go (1)
GetMarkdownRenderer(21-28)
packages/tui/internal/tui/report_controller.go (1)
packages/tui/internal/tui/app_types.go (2)
Model(49-88)DashboardScreen(32-32)
packages/tui/internal/tui/app.go (6)
packages/tui/internal/components/spinner.go (1)
SpinnerTickMsg(11-13)packages/tui/internal/tui/app_types.go (4)
AutoRefreshMsg(11-11)HealthCheckResultMsg(14-17)StartEvaluationMsg(20-20)SummaryGeneratedMsg(23-26)packages/tui/internal/components/command_line.go (1)
CommandSelectedMsg(33-35)packages/tui/internal/components/dialog.go (2)
DialogOpenMsg(60-62)DialogClosedMsg(54-57)packages/tui/internal/components/llm_config_dialog.go (2)
LLMConfigResultMsg(153-158)LLMDialogClosedMsg(161-163)packages/tui/internal/components/scenario_types.go (7)
StartInterviewMsg(42-42)SendInterviewMessageMsg(63-66)InterviewStartedMsg(44-48)InterviewResponseMsg(50-55)GenerateScenariosMsg(68-70)ScenariosGeneratedMsg(57-61)ScenarioEditorMsg(30-33)
packages/tui/internal/tui/eval_form_controller.go (2)
packages/tui/internal/tui/app_types.go (1)
Model(49-88)packages/tui/internal/components/llm_config_dialog.go (1)
NewLLMConfigDialog(183-270)
packages/tui/internal/tui/help_controller.go (1)
packages/tui/internal/tui/app_types.go (1)
Model(49-88)
packages/tui/internal/tui/common_controller.go (8)
packages/tui/internal/tui/app_types.go (12)
Model(49-88)NewEvaluationScreen(35-35)ScenariosScreen(39-39)AutoRefreshMsg(11-11)EvaluationDetailScreen(34-34)HealthCheckResultMsg(14-17)StartEvaluationMsg(20-20)SummaryGeneratedMsg(23-26)ConfigurationScreen(38-38)ConfigState(119-127)HelpScreen(40-40)DashboardScreen(32-32)packages/tui/internal/components/spinner.go (1)
SpinnerTickMsg(11-13)packages/tui/internal/components/dialog_helpers.go (1)
ShowErrorDialog(6-19)packages/tui/internal/components/dialog.go (7)
NewInfoDialog(65-78)NewConfirmationDialog(81-95)DialogOpenMsg(60-62)Dialog(38-51)DialogClosedMsg(54-57)NewInputDialog(98-113)DialogButton(22-26)packages/tui/internal/components/command_line.go (2)
CommandSelectedMsg(33-35)Command(13-18)packages/tui/internal/tui/eval_ui.go (1)
EvaluationViewState(11-36)packages/tui/internal/components/llm_config_dialog.go (2)
NewLLMConfigDialog(183-270)LLMConfigResultMsg(153-158)packages/tui/internal/components/scenario_types.go (7)
StartInterviewMsg(42-42)SendInterviewMessageMsg(63-66)InterviewStartedMsg(44-48)InterviewResponseMsg(50-55)GenerateScenariosMsg(68-70)ScenariosGeneratedMsg(57-61)ScenarioEditorMsg(30-33)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Cursor Bugbot
- GitHub Check: rogue_sanity
- GitHub Check: codestyle
- GitHub Check: test (3.10)
- GitHub Check: Import Time Gate
🔇 Additional comments (7)
packages/tui/internal/tui/dashboard_controller.go (1)
3-4: OK to keep minimal; ensure it’s wired in router.No issues. Confirm keyboard router dispatches to this screen; otherwise this file can be removed until needed.
packages/tui/internal/tui/app_types.go (1)
7-8: The import path is correct; this review comment is based on incorrect assumptions.The module is defined as
github.com/rogue/tuiin packages/tui/go.mod. When go.mod is located at packages/tui/, all packages within that directory are imported relative to the module root. Therefore, packages/tui/internal/components correctly maps to the import pathgithub.com/rogue/tui/internal/components, which properly respects Go's internal package visibility rules. The import is used consistently across 9 files in the codebase and requires no changes.Likely an incorrect or invalid review comment.
packages/tui/internal/tui/report_controller.go (1)
11-13: Early nil-guard blocks Back key on ReportScreen.If evalState can be nil, this early return prevents "b" from navigating back. Confirm this cannot trap the user on ReportScreen; otherwise, move the guard inside specific cases that need evalState.
packages/tui/internal/tui/app_commands.go (2)
68-76: Possible incorrect GenerateSummary arguments.The last argument repeats judgeModel. Verify the expected signature; this may be a copy/paste error.
12-17: autoRefresh tick period OK.500ms tick is a reasonable default for lightweight UI refresh.
packages/tui/internal/tui/keyboard_controller.go (1)
27-34: Dialog handling order looks correct.LLM and general dialogs get first dibs on input. Good layering.
packages/tui/internal/tui/common_controller.go (1)
200-207: Confirm default model choice.Judge default is "openai/gpt-4.1" while interview/scenario defaults use "openai/gpt-4o". Is this intentional?
| // Create rogue config directory if it doesn't exist | ||
| rogueConfigDir := filepath.Join(configDir, "rogue") | ||
| if err := os.MkdirAll(rogueConfigDir, 0755); err != nil { | ||
| return fmt.Errorf("failed to create config directory: %w", err) | ||
| } | ||
|
|
||
| // Config file path | ||
| configFile := filepath.Join(rogueConfigDir, "config.toml") | ||
|
|
||
| // Marshal config to TOML | ||
| data, err := toml.Marshal(m.config) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to marshal config: %w", err) | ||
| } | ||
|
|
||
| // Write to file | ||
| if err := os.WriteFile(configFile, data, 0644); err != nil { | ||
| return fmt.Errorf("failed to write config file: %w", err) | ||
| } |
There was a problem hiding this comment.
Secrets in config; tighten permissions and write atomically.
Config includes API keys. Creating dirs with 0755 and writing files 0644 makes secrets world-readable on multi-user systems. Use 0700 dir, 0600 file, and atomic rename.
- rogueConfigDir := filepath.Join(configDir, "rogue")
- if err := os.MkdirAll(rogueConfigDir, 0755); err != nil {
+ rogueConfigDir := filepath.Join(configDir, "rogue")
+ if err := os.MkdirAll(rogueConfigDir, 0700); err != nil {
return fmt.Errorf("failed to create config directory: %w", err)
}
@@
- // Write to file
- if err := os.WriteFile(configFile, data, 0644); err != nil {
- return fmt.Errorf("failed to write config file: %w", err)
- }
+ // Atomic write with restricted perms
+ tmp, err := os.CreateTemp(rogueConfigDir, "config-*.toml")
+ if err != nil {
+ return fmt.Errorf("failed to create temp config file: %w", err)
+ }
+ defer os.Remove(tmp.Name())
+ if _, err := tmp.Write(data); err != nil {
+ _ = tmp.Close()
+ return fmt.Errorf("failed to write temp config file: %w", err)
+ }
+ if err := tmp.Chmod(0600); err != nil {
+ _ = tmp.Close()
+ return fmt.Errorf("failed to set config file permissions: %w", err)
+ }
+ if err := tmp.Close(); err != nil {
+ return fmt.Errorf("failed to close temp config file: %w", err)
+ }
+ if err := os.Rename(tmp.Name(), configFile); err != nil {
+ return fmt.Errorf("failed to replace config file: %w", err)
+ }Optionally, validate existing file perms during load and warn if too permissive.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Create rogue config directory if it doesn't exist | |
| rogueConfigDir := filepath.Join(configDir, "rogue") | |
| if err := os.MkdirAll(rogueConfigDir, 0755); err != nil { | |
| return fmt.Errorf("failed to create config directory: %w", err) | |
| } | |
| // Config file path | |
| configFile := filepath.Join(rogueConfigDir, "config.toml") | |
| // Marshal config to TOML | |
| data, err := toml.Marshal(m.config) | |
| if err != nil { | |
| return fmt.Errorf("failed to marshal config: %w", err) | |
| } | |
| // Write to file | |
| if err := os.WriteFile(configFile, data, 0644); err != nil { | |
| return fmt.Errorf("failed to write config file: %w", err) | |
| } | |
| // Create rogue config directory if it doesn't exist | |
| rogueConfigDir := filepath.Join(configDir, "rogue") | |
| if err := os.MkdirAll(rogueConfigDir, 0700); err != nil { | |
| return fmt.Errorf("failed to create config directory: %w", err) | |
| } | |
| // Config file path | |
| configFile := filepath.Join(rogueConfigDir, "config.toml") | |
| // Marshal config to TOML | |
| data, err := toml.Marshal(m.config) | |
| if err != nil { | |
| return fmt.Errorf("failed to marshal config: %w", err) | |
| } | |
| // Atomic write with restricted perms | |
| tmp, err := os.CreateTemp(rogueConfigDir, "config-*.toml") | |
| if err != nil { | |
| return fmt.Errorf("failed to create temp config file: %w", err) | |
| } | |
| defer os.Remove(tmp.Name()) | |
| if _, err := tmp.Write(data); err != nil { | |
| _ = tmp.Close() | |
| return fmt.Errorf("failed to write temp config file: %w", err) | |
| } | |
| if err := tmp.Chmod(0600); err != nil { | |
| _ = tmp.Close() | |
| return fmt.Errorf("failed to set config file permissions: %w", err) | |
| } | |
| if err := tmp.Close(); err != nil { | |
| return fmt.Errorf("failed to close temp config file: %w", err) | |
| } | |
| if err := os.Rename(tmp.Name(), configFile); err != nil { | |
| return fmt.Errorf("failed to replace config file: %w", err) | |
| } |
🤖 Prompt for AI Agents
In packages/tui/internal/tui/app_config.go around lines 19 to 37, the config
directory and file are created with permissive 0755/0644 modes exposing secrets;
change MkdirAll to create the rogue config dir with 0700, and instead of writing
directly call a safe atomic write: create a temp file in the same directory,
write the TOML bytes to it, fsync/close, then os.Rename the temp file to
config.toml and set its mode to 0600 to ensure only the owner can read/write;
additionally, when loading configs add an optional permission check that warns
or fixes files whose mode is more permissive than 0600.
yuval-qf
left a comment
yuval-qf
left a comment
There was a problem hiding this comment.
LGTM
This is organized much better now!
Description
Motivation and Context
Type of Change
Changes Made
Screenshots/Examples (if applicable)
Checklist
uv run black .to format my codeuv run flake8 .and fixed all issuesuv run mypy --config-file .mypy.ini .and addressed type checking issuesuv run bandit -c .bandit.yaml -r .for security checksuv run pytestand all tests passTesting
Test Configuration:
Test Steps:
1.
2.
3.
Additional Notes
Related Issues/PRs