refactor(app): remove detach mode

[K1ngst0m]

User description

• Drop --detach flag; users launch apps with target command after --
• Simplify launch documentation and script examples
• Archive previous "default + detach" design proposal
• Reduce CLI complexity and option dependencies

---

PR Type

Enhancement

---

Description

• Remove --detach flag; simplify CLI by requiring app command after --

• Eliminate detach mode validation logic and option dependencies

• Unconditionally launch target app in default mode (no viewer-only option)

• Update documentation and test cases to reflect simplified launch workflow

---

Diagram Walkthrough

flowchart LR
  CLI["CLI Parser"] -->|Remove --detach flag| Options["CliOptions struct"]
  Options -->|Always require app command| Validation["Validation Logic"]
  Validation -->|Eliminate mode branches| Launch["App Launch"]
  Launch -->|Always spawn target app| Runtime["Runtime Execution"]
  Docs["Documentation & Scripts"] -->|Update examples| Simplified["Simplified UX"]

Loading

File Walkthrough

	Relevant files
Enhancement	4 files cli.cpp Remove detach flag and simplify validation                              +11/-39  cli.hpp Remove detach boolean field from struct                                    +0/-1      main.cpp Eliminate detach mode branching in app launch                        +34/-38  profile.sh Remove detach mode validation check                                            +0/-4
Tests	1 files test_cli.cpp Remove detach mode test cases                                                        +0/-32
Documentation	6 files start.sh Update usage docs and remove detach examples                          +1/-5      tasks.md Mark smoke test task as completed                                                +1/-1      design.md Archive design notes for removed feature                                  +0/-43    proposal.md Archive proposal document for removed feature                        +0/-41    spec.md Archive specification for removed feature                                +0/-38    tasks.md Archive task checklist for removed feature                              +0/-37

---

Summary by CodeRabbit

• Documentation

  • Removed archived documentation for launch modes feature.

• Bug Fixes

  • Completed verification of headless mode smoke test.

• Chores

  • Removed detach mode (--detach flag) and related functionality from the application. Updated CLI help text and removed associated validation logic and tests.

[coderabbitai]

📝 Walkthrough

Walkthrough

The changes remove the --detach flag and all associated launch mode functionality from the Goggles application. This includes removing CLI parsing logic, validation constraints, main application loop control flow handling detach mode, task script references, and test coverage. Design and proposal documentation for the launch modes feature are archived and deleted.

Changes

Cohort / File(s)	Summary
Headless Mode Verification openspec/changes/add-headless-mode/tasks.md	Marked the headless smoke test verification item as complete (checkbox state changed from [ ] to [x]).
Launch Modes Archive Removal openspec/changes/archive/2026-02-07-update-launch-modes-default-detach/design.md, proposal.md, specs/app-window/spec.md, tasks.md	Deleted all design, proposal, specification, and task documentation related to the launch modes feature, including default/detach mode behavior definitions, environment variable handling, width/height constraints, and verification checklists.
Task Scripts scripts/task/profile.sh, scripts/task/start.sh	Removed --detach guard logic from profile.sh that previously aborted when detach was present; updated start.sh help text to remove detach workflow documentation and examples, showing only the unified command syntax.
CLI Parsing src/app/cli.hpp, src/app/cli.cpp	Removed detach field from CliOptions struct; eliminated --detach flag registration, detach-mode validation logic, detach-specific error messages, and mutual exclusion checks with --headless.
Main Application Loop src/app/main.cpp	Removed conditional logic gating target app launch on detach mode; target app now launches unconditionally. Updated post-loop control flow to always handle app termination and exit status propagation without detach-mode branches.
CLI Tests tests/app/test_cli.cpp	Removed tests validating detach mode behavior, width/height constraints in detach mode, and mutual exclusivity between --headless and --detach flags.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

• Update Launch UX With Default + Detach Modes #36 — Directly reverts/removes the same --detach flag changes across CLI, main loop, scripts, docs, and tests
• feat(app): refactor CLI parsing #57 — Refactors CLI parsing surface (cli.hpp/cpp, main.cpp, tests) that overlaps with detach option removal paths
• refactor(capture): drop vk layer capture in favor of compositor #95 — Extensive launch-mode and CLI refactor touching the same source files (cli.hpp/cpp, main.cpp, scripts, tests)

Suggested labels

Review effort 3/5

Poem

🐰 No more detaching, just one way to go,
Simpler modes now, a cleaner code flow,
Default mode reigns where detach used to stay,
The rabbit hops forward, removed the other way! ✨

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 20.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title 'refactor(app): remove detach mode' directly and accurately summarizes the main change: removal of the detach mode feature across the codebase, scripts, and documentation.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

• 📝 Generate docstrings (stacked PR)
• 📝 Generate docstrings (commit on current branch)

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch dev/drop-detach

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[qodo-code-review]

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
🟢	No security concerns identified No security vulnerabilities detected by AI analysis. Human verification advised for critical code.
Ticket Compliance
⚪	🎫 No ticket provided Create ticket/issue
Codebase Duplication Compliance
⚪	Codebase context is not defined Follow the guide to enable codebase context checks.
Custom Compliance
🟢	Generic: Meaningful Naming and Self-Documenting Code Objective: Ensure all identifiers clearly express their purpose and intent, making code self-documenting Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Robust Error Handling and Edge Case Management Objective: Ensure comprehensive error handling that provides meaningful context and graceful degradation Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Secure Error Handling Objective: To prevent the leakage of sensitive system information through error messages while providing sufficient detail for internal debugging. Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Security-First Input Validation and Data Handling Objective: Ensure all data inputs are validated, sanitized, and handled securely to prevent vulnerabilities Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
⚪	Generic: Comprehensive Audit Trails Objective: To create a detailed and reliable record of critical system actions for security analysis and compliance. Status: Incomplete audit context: The newly added target-app launch/termination logs record outcomes but do not show inclusion of user identity and explicit timestamps in the log fields, which may be required to reconstruct security-relevant actions. Referred Code GOGGLES_LOG_CRITICAL("Failed to launch target app: {} ({})", spawn_result.error().message, goggles::error_code_name(spawn_result.error().code)); return EXIT_FAILURE; } child_pid = spawn_result.value(); GOGGLES_LOG_INFO("Launched target app (pid={})", child_pid); while (app->is_running()) { app->process_event(); app->tick_frame(); if (child_pid > 0 && !child_exited) { pid_t result = waitpid(child_pid, &child_status, WNOHANG); if (result == child_pid) { child_exited = true; push_quit_event(); } } } ... (clipped 4 lines) Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Secure Logging Practices Objective: To ensure logs are useful for debugging and auditing without exposing sensitive information like PII, PHI, or cardholder data. Status: Log format unclear: The new GOGGLES_LOG_INFO/GOGGLES_LOG_CRITICAL entries appear as formatted text in the diff and it is not verifiable here whether the logging backend emits structured (e.g., JSON) logs as required. Referred Code GOGGLES_LOG_CRITICAL("Failed to launch target app: {} ({})", spawn_result.error().message, goggles::error_code_name(spawn_result.error().code)); return EXIT_FAILURE; } child_pid = spawn_result.value(); GOGGLES_LOG_INFO("Launched target app (pid={})", child_pid); Learn more about managing compliance generic rules or creating your own custom rules

Compliance status legend

🟢 - Fully Compliant
🟡 - Partial Compliant
🔴 - Not Compliant
⚪ - Requires Further Human Verification
🏷️ - Compliance label

[qodo-code-review]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
General	Improve child process exit code handling Improve child process exit handling by checking if it was terminated by a signal (WIFSIGNALED). If so, log the signal and return a conventional exit code (128 + signal number) for better diagnostics. src/app/main.cpp [538-543] if (child_pid > 0 && child_exited) { if (WIFEXITED(child_status)) { return WEXITSTATUS(child_status); } + if (WIFSIGNALED(child_status)) { + GOGGLES_LOG_INFO("Target app terminated by signal {}", WTERMSIG(child_status)); + return 128 + WTERMSIG(child_status); + } return EXIT_FAILURE; } Apply / Chat Suggestion importance[1-10]: 6 __ Why: The suggestion correctly identifies that handling child process termination by signal more gracefully would improve diagnostics, which is a valuable enhancement for usability in scripted environments.	Low
	Improve parse error clarity Improve the CLI parsing error messages for missing commands or separators to be more explicit and consistently formatted. src/app/cli.cpp [99-101] (argc <= 1) - ? "missing target app command (pass app after '--')" - : "missing '--' separator before target app command"); + ? "Missing target application; use `-- <app> [app_args...]`." + : "Missing `--` separator before the target application."; [To ensure code accuracy, apply this suggestion manually] Suggestion importance[1-10]: 2 __ Why: The suggestion offers a minor improvement to an error message for better clarity, which is a stylistic change with low impact on functionality.	Low
More

[coderabbitai]

🧹 Nitpick comments (1)

src/app/main.cpp (1)

532-547: Unreachable code path after child process handling.

After the loop exits, child_pid is always > 0 (spawn succeeded at line 516, or we returned early at line 514). The control flow handles both !child_exited (lines 532-536) and child_exited (lines 538-543), both of which return. Therefore, lines 544-547 ("Shutting down..." and EXIT_SUCCESS) are unreachable.

Consider removing the unreachable code for clarity:

♻️ Suggested cleanup

         if (child_pid > 0 && child_exited) {
             if (WIFEXITED(child_status)) {
                 return WEXITSTATUS(child_status);
             }
             return EXIT_FAILURE;
         }
-        GOGGLES_LOG_INFO("Shutting down...");
     }
     GOGGLES_LOG_INFO("Goggles terminated successfully");
     return EXIT_SUCCESS;
 }

Or alternatively, if you want to keep the success path for future extensibility, mark it explicitly:

// Note: Currently unreachable; kept for future modes that may not spawn a child
GOGGLES_LOG_INFO("Shutting down...");

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/app/main.cpp` around lines 532 - 547, The final
GOGGLES_LOG_INFO("Shutting down...") and return EXIT_SUCCESS are unreachable
because after spawning the child (child_pid) the branches handling !child_exited
(calling terminate_child(child_pid) and returning EXIT_FAILURE) and child_exited
(checking WIFEXITED(child_status) and returning WEXITSTATUS or EXIT_FAILURE)
always return; remove the unreachable GOGGLES_LOG_INFO and EXIT_SUCCESS lines,
or if you want to keep them for future modes, convert them into an explicit
commented note above the log (e.g., "// Currently unreachable; kept for future
non-child modes") so the intent is clear while leaving control flow unchanged —
update references in main.cpp around child_pid, child_exited, WIFEXITED,
child_status, and terminate_child accordingly.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@src/app/main.cpp`:
- Around line 532-547: The final GOGGLES_LOG_INFO("Shutting down...") and return
EXIT_SUCCESS are unreachable because after spawning the child (child_pid) the
branches handling !child_exited (calling terminate_child(child_pid) and
returning EXIT_FAILURE) and child_exited (checking WIFEXITED(child_status) and
returning WEXITSTATUS or EXIT_FAILURE) always return; remove the unreachable
GOGGLES_LOG_INFO and EXIT_SUCCESS lines, or if you want to keep them for future
modes, convert them into an explicit commented note above the log (e.g., "//
Currently unreachable; kept for future non-child modes") so the intent is clear
while leaving control flow unchanged — update references in main.cpp around
child_pid, child_exited, WIFEXITED, child_status, and terminate_child
accordingly.

---

ℹ️ Review info

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 0550ca7 and b435285.

📒 Files selected for processing (11)

• openspec/changes/add-headless-mode/tasks.md
• openspec/changes/archive/2026-02-07-update-launch-modes-default-detach/design.md
• openspec/changes/archive/2026-02-07-update-launch-modes-default-detach/proposal.md
• openspec/changes/archive/2026-02-07-update-launch-modes-default-detach/specs/app-window/spec.md
• openspec/changes/archive/2026-02-07-update-launch-modes-default-detach/tasks.md
• scripts/task/profile.sh
• scripts/task/start.sh
• src/app/cli.cpp
• src/app/cli.hpp
• src/app/main.cpp
• tests/app/test_cli.cpp

💤 Files with no reviewable changes (7)

• openspec/changes/archive/2026-02-07-update-launch-modes-default-detach/tasks.md
• src/app/cli.hpp
• openspec/changes/archive/2026-02-07-update-launch-modes-default-detach/specs/app-window/spec.md
• tests/app/test_cli.cpp
• openspec/changes/archive/2026-02-07-update-launch-modes-default-detach/design.md
• openspec/changes/archive/2026-02-07-update-launch-modes-default-detach/proposal.md
• scripts/task/profile.sh