Conversation
- Add CliAction/CliParseOutcome and return exit_ok on --help/--version - Remove ErrorCode::ok sentinel from the CLI parse error path - Move CLI11 parsing out of src/app/cli.hpp into src/app/cli.cpp - Update src/app/main.cpp to handle exit_ok as EXIT_SUCCESS - Update build + tests to compile/cover the new CLI path (incl. help/version)
📝 WalkthroughWalkthroughThis PR refactors CLI parsing by introducing an explicit Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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. 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: 1
🤖 Fix all issues with AI agents
In `@src/app/cli.cpp`:
- Around line 140-162: The catch block for CLI::ParseError currently
short-circuits to a "missing '--' separator" message when !has_separator, which
hides the actual parse error; update the CLI::ParseError handling in the
try/catch around app.parse(viewer_argc, argv) to remove the early return for the
!has_separator branch and instead log/return the real parse error using e.what()
(e.g., replace the make_error<CliParseOutcome>(..., "missing '--'...") path with
a make_error that includes e.what()), still calling app.exit(e) as done
currently; leave validate_default_mode() to handle separator-specific
validation.
🧹 Nitpick comments (1)
openspec/changes/refactor-cli-parse-outcome/proposal.md (1)
34-38: Consider varying sentence structure in the Non-Goals section.Three consecutive items begin with "Changing", which reads somewhat repetitively. Consider rephrasing for variety, e.g., "Launch-mode behavior changes beyond..." for the third item.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
openspec/changes/refactor-cli-parse-outcome/design.mdopenspec/changes/refactor-cli-parse-outcome/proposal.mdopenspec/changes/refactor-cli-parse-outcome/specs/app-window/spec.mdopenspec/changes/refactor-cli-parse-outcome/tasks.mdsrc/app/CMakeLists.txtsrc/app/cli.cppsrc/app/cli.hppsrc/app/main.cpptests/CMakeLists.txttests/app/test_cli.cpp
🧰 Additional context used
📓 Path-based instructions (3)
openspec/changes/**/specs/**/*.md
📄 CodeRabbit inference engine (openspec/AGENTS.md)
openspec/changes/**/specs/**/*.md: Write spec deltas using## ADDED|MODIFIED|REMOVED|RENAMED Requirementsformat with at least one#### Scenario:per requirement in spec files
When modifying existing requirements in a MODIFIED delta, paste the full requirement block including the header and all scenarios, as the archiver will replace the entire requirement
Use ADDED for new capabilities that can stand alone; use MODIFIED for changes to existing requirement behavior, scope, or acceptance criteria; use RENAMED for name-only changes
Files:
openspec/changes/refactor-cli-parse-outcome/specs/app-window/spec.md
openspec/**/*.md
📄 CodeRabbit inference engine (openspec/AGENTS.md)
openspec/**/*.md: Use#### Scenario:format (4 hashtags) for scenario headers in requirements, not bullets or bold text, with at least one scenario per requirement
Use format- **WHEN** [condition] - **THEN** [expected result]for scenario steps in requirements
Files:
openspec/changes/refactor-cli-parse-outcome/specs/app-window/spec.mdopenspec/changes/refactor-cli-parse-outcome/tasks.mdopenspec/changes/refactor-cli-parse-outcome/design.mdopenspec/changes/refactor-cli-parse-outcome/proposal.md
openspec/changes/**/proposal.md
📄 CodeRabbit inference engine (openspec/AGENTS.md)
List spec deltas in
proposal.mdunder 'What Changes' section, marking breaking changes with BREAKING
Files:
openspec/changes/refactor-cli-parse-outcome/proposal.md
🧠 Learnings (14)
📚 Learning: 2025-12-19T08:54:18.412Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: openspec/AGENTS.md:0-0
Timestamp: 2025-12-19T08:54:18.412Z
Learning: Applies to openspec/changes/**/specs/**/*.md : Use ADDED for new capabilities that can stand alone; use MODIFIED for changes to existing requirement behavior, scope, or acceptance criteria; use RENAMED for name-only changes
Applied to files:
openspec/changes/refactor-cli-parse-outcome/specs/app-window/spec.md
📚 Learning: 2025-12-19T08:54:18.412Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: openspec/AGENTS.md:0-0
Timestamp: 2025-12-19T08:54:18.412Z
Learning: Applies to openspec/changes/**/specs/**/*.md : Write spec deltas using `## ADDED|MODIFIED|REMOVED|RENAMED Requirements` format with at least one `#### Scenario:` per requirement in spec files
Applied to files:
openspec/changes/refactor-cli-parse-outcome/specs/app-window/spec.md
📚 Learning: 2026-01-16T06:42:50.935Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-16T06:42:50.935Z
Learning: Applies to docs/tests/**/*.cpp : Use Catch2 v3 as the project-wide testing framework. Test only non-GPU logic (utility functions, configuration parsing, error handling, pipeline graph logic). GPU/Vulkan testing deferred. Test directory structure mirrors `src/`. Test files named `test_<module>.cpp`. Test cases use descriptive names.
Applied to files:
tests/CMakeLists.txttests/app/test_cli.cpp
📚 Learning: 2025-12-29T15:44:30.520Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: openspec/project.md:0-0
Timestamp: 2025-12-29T15:44:30.520Z
Learning: Applies to openspec/tests/**/*.{cpp,hpp} : Focus unit tests on non-GPU logic: utility functions, configuration parsing, and pipeline graph logic
Applied to files:
tests/CMakeLists.txttests/app/test_cli.cpp
📚 Learning: 2025-12-29T15:44:30.520Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: openspec/project.md:0-0
Timestamp: 2025-12-29T15:44:30.520Z
Learning: Applies to openspec/tests/**/*.{cpp,hpp} : Use Catch2 v3 for unit testing with test files mirroring the `src/` directory structure
Applied to files:
tests/CMakeLists.txt
📚 Learning: 2026-01-16T06:42:50.935Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-16T06:42:50.935Z
Learning: Applies to docs/**/*.{cpp,hpp} : Use namespaces: top-level `goggles`, modules as `goggles::<module>` (e.g., goggles::capture, goggles::render), sub-modules as `goggles::<module>::<submodule>` (e.g., goggles::capture::vk_layer). Never use `using namespace` in headers; prefer explicit qualification or scoped `using`.
Applied to files:
tests/CMakeLists.txtsrc/app/cli.hpp
📚 Learning: 2025-12-29T15:44:30.520Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: openspec/project.md:0-0
Timestamp: 2025-12-29T15:44:30.520Z
Learning: Applies to openspec/**/*.hpp : Organize namespaces as `goggles` with sub-modules like `goggles::capture`, `goggles::render`; never use `using namespace` in headers
Applied to files:
tests/CMakeLists.txtsrc/app/cli.hpp
📚 Learning: 2026-01-16T06:42:50.935Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-16T06:42:50.935Z
Learning: Applies to docs/**/*.{cpp,hpp} : Define a lightweight `Error` struct with `ErrorCode` enum, `std::string message`, and optional `std::source_location loc` in the `goggles` namespace.
Applied to files:
src/app/cli.hpp
📚 Learning: 2026-01-16T06:42:50.935Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-16T06:42:50.935Z
Learning: Applies to docs/**/*.{cpp,hpp} : Use project-specific logging macros (GOGGLES_LOG_TRACE, GOGGLES_LOG_DEBUG, GOGGLES_LOG_INFO, GOGGLES_LOG_WARN, GOGGLES_LOG_ERROR, GOGGLES_LOG_CRITICAL) wrapping spdlog with severity levels: trace, debug, info, warn, error, critical.
Applied to files:
src/app/cli.hpp
📚 Learning: 2026-01-16T06:42:50.935Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-16T06:42:50.935Z
Learning: Applies to docs/**/*.{cpp,hpp} : All vulkan-hpp calls returning `vk::Result` must be explicitly checked. Do not use `static_cast<void>` to suppress results. Use `VK_TRY` macro for early return, `GOGGLES_TRY` for Result<T> propagation, `GOGGLES_MUST` for internal invariants. Manual error handling with `if (result != vk::Result::eSuccess)` for complex cleanup.
Applied to files:
src/app/cli.hpp
📚 Learning: 2025-12-29T15:44:30.520Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: openspec/project.md:0-0
Timestamp: 2025-12-29T15:44:30.520Z
Learning: Applies to openspec/src/**/*.{cpp,hpp} : Exceptions are only for unrecoverable programming errors; use `tl::expected` for recoverable errors
Applied to files:
src/app/cli.hpp
📚 Learning: 2026-01-16T06:42:50.935Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-16T06:42:50.935Z
Learning: Applies to docs/**/*.{cpp,hpp} : All fallible operations must return `tl::expected<T, Error>` instead of using exceptions for expected failures (file I/O, resource creation, parsing).
Applied to files:
src/app/cli.hpp
📚 Learning: 2025-12-29T15:44:30.520Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: openspec/project.md:0-0
Timestamp: 2025-12-29T15:44:30.520Z
Learning: Applies to openspec/src/**/*.{cpp,hpp} : All fallible operations MUST return `tl::expected<T, Error>` for error handling
Applied to files:
src/app/cli.hpp
📚 Learning: 2026-01-16T06:42:50.935Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-16T06:42:50.935Z
Learning: Applies to docs/src/capture/vk_layer/**/*.{cpp,hpp} : Vulkan capture layer logging: default is off. Enable with `GOGGLES_DEBUG_LOG=1`. Configure level with `GOGGLES_DEBUG_LOG_LEVEL=trace|debug|info|warn|error|critical|off` (default: info). No file I/O in hot paths. Prefix all logs with `[goggles_vklayer]`. Never log in `vkQueuePresentKHR` hot path.
Applied to files:
tests/app/test_cli.cpp
🧬 Code graph analysis (3)
src/app/cli.cpp (1)
src/util/error.hpp (1)
make_error(54-56)
src/app/cli.hpp (2)
src/app/cli.cpp (5)
nodiscard(8-13)nodiscard(15-17)nodiscard(19-26)nodiscard(61-84)nodiscard(86-111)tests/app/test_cli.cpp (2)
nodiscard(22-22)nodiscard(25-27)
tests/app/test_cli.cpp (2)
src/app/cli.cpp (2)
parse_cli(121-187)parse_cli(121-121)src/app/cli.hpp (1)
parse_cli(41-41)
🪛 LanguageTool
openspec/changes/refactor-cli-parse-outcome/proposal.md
[style] ~38-~38: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...nonstd::expectedvstl::expected`). - Changing launch-mode behavior beyond improving v...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
⏰ 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). (2)
- GitHub Check: Static Analysis (clang-tidy)
- GitHub Check: Build and Test (test preset)
🔇 Additional comments (19)
src/app/CMakeLists.txt (1)
3-9: LGTM!The addition of
cli.cppto the source list correctly integrates the new CLI parsing implementation file with the build. TheCLI11::CLI11library is already linked, so no additional linkage changes are needed.tests/CMakeLists.txt (2)
15-19: LGTM!Including
cli.cppdirectly in the test sources is a reasonable approach for testing the CLI parsing implementation without creating a separate library target.
55-61: LGTM!Adding
CLI11::CLI11to the test target's link libraries is necessary since the CLI parsing implementation depends on CLI11.openspec/changes/refactor-cli-parse-outcome/proposal.md (1)
1-46: Proposal document structure looks good.The document follows the expected format with clear sections for motivation, changes, impact, non-goals, and open questions. The "What Changes" section appropriately lists the spec deltas without requiring BREAKING markers since no breaking changes are introduced. Based on coding guidelines.
src/app/main.cpp (2)
324-328: LGTM!The new outcome-based flow cleanly separates the "exit successfully" case (
--help/--version) from the normal run path. ReturningEXIT_SUCCESSimmediately forexit_okavoids unnecessary initialization (logger, config loading, etc.) and correctly reflects that the CLI request was handled successfully.
317-328: Clean separation of parse failure vs. exit-ok vs. run.The three-way branching is well-structured:
- Parse failure (lines 319-323): Print error and exit with failure
- Exit-ok (lines 325-327): Return success without further work
- Run (line 328+): Extract options and proceed with application
This is a meaningful improvement over the previous
ErrorCode::oksentinel pattern.openspec/changes/refactor-cli-parse-outcome/specs/app-window/spec.md (1)
1-29: Spec delta document follows the required format.The document correctly uses:
## MODIFIED Requirementsheader (appropriate since this changes existing CLI behavior, not adding new capabilities)#### Scenario:format for all three scenarios- **WHEN** ... - **THEN** ...format for scenario stepsThe scenarios clearly document that "exit successfully" disposition replaces the previous error sentinel approach. Based on coding guidelines and learnings.
src/app/cli.hpp (1)
29-41: Clear outcome-based API surface.The action + outcome wrapper makes the parse contract straightforward for callers.
openspec/changes/refactor-cli-parse-outcome/tasks.md (1)
1-19: Checklist matches the PR scope and validation steps.src/app/cli.cpp (7)
8-13: Exit-ok helper is concise and explicit.
15-26: Separator helpers are straightforward and safe.
28-59: Option wiring is centralized and readable.
61-84: Detach-mode validation rules are clear and consistent.
86-111: Default-mode validation logic is clean and self-contained.
113-117: Normalization step is concise and easy to reason about.
121-139: Parse flow reads well and mirrors the outcome model.Also applies to: 164-187
openspec/changes/refactor-cli-parse-outcome/design.md (1)
18-63: Design notes clearly match the implemented refactor.tests/app/test_cli.cpp (2)
35-39: Updated assertions consistently validate the new outcome model.Also applies to: 92-99, 108-114, 121-125, 132-137
148-162: Good coverage for--help/--versionsuccess paths.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| try { | ||
| app.parse(viewer_argc, argv); | ||
| } catch (const CLI::ParseError& e) { | ||
| if (e.get_exit_code() == 0) { | ||
| (void)app.exit(e); | ||
| return make_exit_ok(); | ||
| } | ||
|
|
||
| if (!has_separator) { | ||
| return make_error<CliParseOutcome>( | ||
| ErrorCode::parse_error, | ||
| (argc <= 1) | ||
| ? "missing target app command (use '--detach' for viewer-only mode, or pass " | ||
| "app " | ||
| "after '--')" | ||
| : "missing '--' separator before target app command (use '--detach' for " | ||
| "viewer-only mode)"); | ||
| } | ||
|
|
||
| (void)app.exit(e); | ||
| return make_error<CliParseOutcome>(ErrorCode::parse_error, | ||
| "Failed to parse command line arguments."); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n src/app/cli.cpp | sed -n '120,180p'Repository: K1ngst0m/Goggles
Length of output: 2827
🏁 Script executed:
rg -n "has_separator" src/app/cli.cpp -B 5 -A 5Repository: K1ngst0m/Goggles
Length of output: 2345
🏁 Script executed:
rg -n "make_error<CliParseOutcome>" src/app/cli.cpp -B 3 -A 3Repository: K1ngst0m/Goggles
Length of output: 959
Don't mask parse errors when -- is absent.
When parsing fails (e.g., missing option value), the !has_separator branch returns a "missing separator" message instead of reporting the actual parse error. This masks the real issue from users. The post-parse validate_default_mode() already checks for missing separators, so remove this early return and let parse errors propagate. Use e.what() to report the actual CLI library error.
🔧 Proposed fix
} catch (const CLI::ParseError& e) {
if (e.get_exit_code() == 0) {
(void)app.exit(e);
return make_exit_ok();
}
-
- if (!has_separator) {
- return make_error<CliParseOutcome>(
- ErrorCode::parse_error,
- (argc <= 1)
- ? "missing target app command (use '--detach' for viewer-only mode, or pass "
- "app "
- "after '--')"
- : "missing '--' separator before target app command (use '--detach' for "
- "viewer-only mode)");
- }
-
(void)app.exit(e);
- return make_error<CliParseOutcome>(ErrorCode::parse_error,
- "Failed to parse command line arguments.");
+ return make_error<CliParseOutcome>(ErrorCode::parse_error, e.what());
}📝 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.
| try { | |
| app.parse(viewer_argc, argv); | |
| } catch (const CLI::ParseError& e) { | |
| if (e.get_exit_code() == 0) { | |
| (void)app.exit(e); | |
| return make_exit_ok(); | |
| } | |
| if (!has_separator) { | |
| return make_error<CliParseOutcome>( | |
| ErrorCode::parse_error, | |
| (argc <= 1) | |
| ? "missing target app command (use '--detach' for viewer-only mode, or pass " | |
| "app " | |
| "after '--')" | |
| : "missing '--' separator before target app command (use '--detach' for " | |
| "viewer-only mode)"); | |
| } | |
| (void)app.exit(e); | |
| return make_error<CliParseOutcome>(ErrorCode::parse_error, | |
| "Failed to parse command line arguments."); | |
| } | |
| try { | |
| app.parse(viewer_argc, argv); | |
| } catch (const CLI::ParseError& e) { | |
| if (e.get_exit_code() == 0) { | |
| (void)app.exit(e); | |
| return make_exit_ok(); | |
| } | |
| (void)app.exit(e); | |
| return make_error<CliParseOutcome>(ErrorCode::parse_error, e.what()); | |
| } |
🤖 Prompt for AI Agents
In `@src/app/cli.cpp` around lines 140 - 162, The catch block for CLI::ParseError
currently short-circuits to a "missing '--' separator" message when
!has_separator, which hides the actual parse error; update the CLI::ParseError
handling in the try/catch around app.parse(viewer_argc, argv) to remove the
early return for the !has_separator branch and instead log/return the real parse
error using e.what() (e.g., replace the make_error<CliParseOutcome>(...,
"missing '--'...") path with a make_error that includes e.what()), still calling
app.exit(e) as done currently; leave validate_default_mode() to handle
separator-specific validation.
1 participant
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.