Add JSON CLI output and publishable ClawHub skill bundle#50
Add JSON CLI output and publishable ClawHub skill bundle#50
Conversation
WalkthroughAdds agent skill documentation and a publishable Changes
Sequence DiagramsequenceDiagram
actor Agent as AI Agent
participant Parser as CLI Parser
participant Handler as CLI Handler
participant Device as Device/Video Layer
Agent->>Parser: run `ccap --list-devices --json`
Parser->>Parser: parse args -> CLIOptions{jsonOutput=true, schemaVersion}
Parser->>Handler: invoke command with CLIOptions
Handler->>Device: query devices / open video
Device-->>Handler: device list / video metadata or error
Handler->>Handler: build JSON envelope (schema_version, command, success, data/error)
Handler->>Agent: emit JSON response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
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📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
There was a problem hiding this comment.
Pull request overview
This PR adds structured JSON output to the ccap CLI (device listing, device info, and video info), expands test coverage for the new JSON contract, and introduces a publishable ClawHub/OpenClaw skill bundle under skills/ccap with repository entrypoints and publishing guidance.
Changes:
- Add
--json+--schema-versionparsing and JSON emitters for--list-devices,--device-info, and video info. - Add/extend CLI parser + integration tests to exercise JSON output paths.
- Add
skills/ccapbundle (SKILL.md + references + publishing docs) and link it from README + AGENTS.md.
Reviewed changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test_cli_args_parser.cpp | Adds a unit test for --json / --schema-version parsing. |
| tests/test_ccap_cli.cpp | Adds integration tests asserting JSON output envelope for devices and video info. |
| skills/ccap/references/install.md | Adds install decision tree notes for the publishable skill. |
| skills/ccap/references/commands.md | Adds agent-oriented command reference emphasizing --json. |
| skills/ccap/SKILL.md | Introduces publishable skill entrypoint and operating rules. |
| skills/ccap/PUBLISHING.md | Documents how to publish the skill folder to ClawHub. |
| skills/README.md | Declares skills/ as the publishable-skill root and sets structural rules. |
| docs/content/cli.zh.md | Documents --json / --schema-version and adds JSON examples (Chinese). |
| docs/content/cli.md | Documents --json / --schema-version and adds JSON examples (English). |
| cli/ccap_cli_utils.h | Updates printVideoInfo signature to accept CLIOptions. |
| cli/ccap_cli_utils.cpp | Implements JSON serialization helpers and JSON output for list/device/video info. |
| cli/ccap_cli.cpp | Wires CLIOptions into printVideoInfo call site. |
| cli/args_parser.h | Adds jsonOutput + schemaVersion to CLIOptions. |
| cli/args_parser.cpp | Adds parsing + usage strings for --json / --schema-version. |
| README.zh-CN.md | Adds entrypoints and publishing guidance for the skill bundle (Chinese). |
| README.md | Adds entrypoints and publishing guidance for the skill bundle (English). |
| AGENTS.md | Adds a repository agent entry directing to skills/ccap. |
docs/content/cli.md
Outdated
| ccap -i /path/to/video.mp4 --json | ||
| ``` | ||
|
|
||
| JSON output currently covers device enumeration, device info, and video metadata. The payload uses a stable top-level envelope with `schema_version`, `command`, `success`, and either `data` or `error`. |
There was a problem hiding this comment.
This section documents the JSON envelope as containing schema_version, command, success, and either data or error, but the implementation currently adds an extra top-level exit_code on errors. Please update this description to match the actual JSON contract (or adjust the implementation to match the doc).
| JSON output currently covers device enumeration, device info, and video metadata. The payload uses a stable top-level envelope with `schema_version`, `command`, `success`, and either `data` or `error`. | |
| JSON output currently covers device enumeration, device info, and video metadata. The payload uses a stable top-level envelope with `schema_version`, `command`, `success`, and either a `data` object on success or an `error` object plus an `exit_code` on failure. |
docs/content/cli.zh.md
Outdated
| ccap -i /path/to/video.mp4 --json | ||
| ``` | ||
|
|
||
| 当前 JSON 输出先覆盖设备枚举、设备信息和视频元数据。返回体采用稳定的顶层结构，包含 `schema_version`、`command`、`success`，以及 `data` 或 `error`。 |
There was a problem hiding this comment.
此处描述 JSON 顶层结构仅包含 schema_version、command、success，以及 data 或 error；但当前实现的错误返回还包含额外的顶层字段 exit_code。请同步更新文档以匹配实际 JSON 合约（或调整实现以匹配文档）。
| 当前 JSON 输出先覆盖设备枚举、设备信息和视频元数据。返回体采用稳定的顶层结构，包含 `schema_version`、`command`、`success`，以及 `data` 或 `error`。 | |
| 当前 JSON 输出先覆盖设备枚举、设备信息和视频元数据。返回体采用稳定的顶层结构，始终包含 `schema_version`、`command`、`success` 字段；成功时携带 `data` 字段，错误时携带 `error` 字段，并额外包含顶层字段 `exit_code`。 |
| // If video file specified without action, print video info | ||
| if (!opts.videoFilePath.empty() && !hasAction) { | ||
| return ccap_cli::printVideoInfo(opts.videoFilePath); | ||
| return ccap_cli::printVideoInfo(opts, opts.videoFilePath); |
There was a problem hiding this comment.
On Linux, the early videoFilePath check returns a plain-text error to stderr and exits before calling printVideoInfo. When --json is set, this breaks the new JSON CLI contract for video-info (and any video-file usage). Consider emitting a JSON error when opts.jsonOutput is true, or moving this platform gate into printVideoInfo so JSON output remains consistent.
| * @brief Print video file information | ||
| * @param videoPath Path to video file | ||
| * @return Exit code (0 on success, 1 on error) | ||
| */ | ||
| int printVideoInfo(const std::string& videoPath); | ||
| int printVideoInfo(const CLIOptions& opts, const std::string& videoPath); |
There was a problem hiding this comment.
The function signature now takes const CLIOptions& opts, but the doc comment still only documents videoPath. Please add an @param opts entry (and clarify which options affect output, e.g., jsonOutput/schemaVersion) so the header stays accurate for callers.
cli/args_parser.cpp
Outdated
| if (i + 1 < argc) { | ||
| opts.schemaVersion = argv[++i]; | ||
| } |
There was a problem hiding this comment.
--schema-version silently does nothing when provided without a following value. Since the usage string documents a required argument, this should be treated as an error (print message + usage and exit non-zero), similar to how --camera-backend enforces its required value.
| if (i + 1 < argc) { | |
| opts.schemaVersion = argv[++i]; | |
| } | |
| if (i + 1 >= argc) { | |
| printWindowsBackendOptionErrorAndExit(argv[0], "--schema-version requires a value."); | |
| } | |
| opts.schemaVersion = argv[++i]; |
| os << ",\"success\":false,\"exit_code\":" << exitCode << ",\"error\":{\"code\":"; | ||
| writeJsonEscapedString(os, code); | ||
| os << ",\"message\":"; | ||
| writeJsonEscapedString(os, message); | ||
| os << "}}"; |
There was a problem hiding this comment.
The JSON error envelope includes an exit_code top-level field, but the docs describe the stable envelope as only schema_version, command, success, and data/error. Either update the documented contract (and keep it consistent across success/error), or remove/relocate exit_code to avoid surprising strict JSON consumers.
| os << ",\"success\":false,\"exit_code\":" << exitCode << ",\"error\":{\"code\":"; | |
| writeJsonEscapedString(os, code); | |
| os << ",\"message\":"; | |
| writeJsonEscapedString(os, message); | |
| os << "}}"; | |
| os << ",\"success\":false,\"error\":{\"code\":"; | |
| writeJsonEscapedString(os, code); | |
| os << ",\"message\":"; | |
| writeJsonEscapedString(os, message); | |
| os << ",\"exit_code\":" << exitCode << "}}"; |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
tests/test_ccap_cli.cpp (1)
388-417: Consider extracting a shared JSON envelope assertion helper.
schema_version/command/successchecks are repeated across new tests; a helper would reduce drift and simplify future schema updates.Also applies to: 753-768
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_ccap_cli.cpp` around lines 388 - 417, Extract a small reusable helper function (e.g., AssertJsonEnvelope or ExpectCliJsonEnvelope) and call it from TEST_F(CCAPCLIDeviceTest, ListDevicesJson) and TEST_F(CCAPCLIDeviceTest, ShowDeviceInfoJson) to centralize the repeated assertions for "\"schema_version\":\"1.0\"", "\"command\":\"...\"", and "\"success\":..."; the helper should accept the CLI output string and expected command name (or allow command to be optional) and perform the common EXPECT_THAT checks, then replace the duplicated checks in those tests (and the similar block around lines 753-768) with a single call to the new helper to reduce duplication and make future schema changes easier to update.cli/ccap_cli_utils.cpp (2)
474-490: Consider guarding against invalid floating-point values in JSON output.The
<<operator fordoublecan producenan,inf, or-inffor special floating-point values, which are not valid JSON numbers per RFC 8259. While unlikely in normal operation, corrupted video files could potentially yield such values.🛡️ Optional: Add validation for floating-point values
+ auto safeDouble = [](double v) -> double { + return std::isfinite(v) ? v : 0.0; + }; + if (opts.jsonOutput) { std::ostringstream os; os << "{\"schema_version\":"; writeJsonEscapedString(os, opts.schemaVersion); os << ",\"command\":"; writeJsonEscapedString(os, "video-info"); os << ",\"success\":true,\"data\":{\"video_path\":"; writeJsonEscapedString(os, videoPath); os << ",\"width\":" << width << ",\"height\":" << height - << ",\"frame_rate\":" << frameRate - << ",\"duration_seconds\":" << duration + << ",\"frame_rate\":" << safeDouble(frameRate) + << ",\"duration_seconds\":" << safeDouble(duration) << ",\"total_frames\":" << static_cast<int>(frameCount) << "}}";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/ccap_cli_utils.cpp` around lines 474 - 490, The JSON builder currently streams floating values (frameRate, duration) directly which can produce NaN/Inf; update the serialization to validate floats with std::isfinite (from <cmath>) before writing: for each floating variable (frameRate, duration) check std::isfinite(value) and if false emit the JSON literal null (or a quoted fallback) instead of the raw value, otherwise emit the numeric value as before; implement a small helper used when building the os stream (referencing opts.jsonOutput logic and the existing writeJsonEscapedString context) so the same validation can be reused and frameCount/width/height handling remains unchanged.
501-509: Redundant(void)opts;cast.The
optsparameter is already used on lines 501-503 (opts.jsonOutputandopts.schemaVersion), making the(void)opts;suppression on line 507 unnecessary.🧹 Remove redundant cast
std::cerr << "Video file playback is not supported. Rebuild with CCAP_ENABLE_FILE_PLAYBACK=ON" << std::endl; - (void)opts; (void)videoPath; return 1;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/ccap_cli_utils.cpp` around lines 501 - 509, Remove the redundant suppression for opts: delete the line "(void)opts;" since opts is already referenced (opts.jsonOutput and opts.schemaVersion) in the surrounding block; keep the unused-videoPath suppression "(void)videoPath;" if needed and leave the calls to printJsonError and the return statements unchanged (references: opts, videoPath, printJsonError).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cli/args_parser.cpp`:
- Around line 349-352: The CLI parser currently accepts "--schema-version"
without a following value and silently continues; update the argument handling
in the parse loop (the block handling arg == "--schema-version" that assigns
opts.schemaVersion = argv[++i]) to validate presence of a value: if i+1 >= argc,
emit a clear error message about the missing schema version and abort with a
non-zero exit (or return an error/throw an exception consistent with existing
parse error handling), otherwise consume argv[++i] into opts.schemaVersion as
before.
In `@docs/content/cli.md`:
- Line 233: The doc's JSON envelope summary omits the top-level exit_code field
produced on error responses; update the envelope description that lists
schema_version, command, success, data/error to explicitly include exit_code (an
integer present on error responses), describe its type and meaning, and show an
example error envelope including "exit_code" alongside "error" so the documented
schema matches the implementation.
In `@docs/content/cli.zh.md`:
- Around line 88-89: The changed lines in docs/content/cli.zh.md introduce
Chinese text for CLI flags (`--json` and `--schema-version VERSION`) and other
sections (lines 194–210) which violate the English-only docs rule; update these
lines to English equivalents (e.g., "`--json` | Output structured JSON for
supported commands" and "`--schema-version VERSION` | Set the schema_version
field in JSON output (default: `1.0`)") or move the Chinese content to a
localized docs directory and keep docs/content/cli.md strictly English; ensure
references to the flags and any headings use the same identifiers (`--json`,
`--schema-version VERSION`) so links/search remain valid.
In `@skills/ccap/references/install.md`:
- Around line 31-35: The docs currently suggest running
./scripts/build_and_install.sh but that script may not enable the CCAP CLI
build; update the install.md text to warn readers that the shortcut may not
produce the ccap binary and add an explicit alternative: instruct users to run a
CMake configure with CCAP_BUILD_CLI=ON and CCAP_INSTALL=ON, then build and
install the project and verify the ccap --version output; reference the
build/install flow and the CCAP_BUILD_CLI and CCAP_INSTALL options and the ccap
executable so readers know exactly which flags and artifact are required.
---
Nitpick comments:
In `@cli/ccap_cli_utils.cpp`:
- Around line 474-490: The JSON builder currently streams floating values
(frameRate, duration) directly which can produce NaN/Inf; update the
serialization to validate floats with std::isfinite (from <cmath>) before
writing: for each floating variable (frameRate, duration) check
std::isfinite(value) and if false emit the JSON literal null (or a quoted
fallback) instead of the raw value, otherwise emit the numeric value as before;
implement a small helper used when building the os stream (referencing
opts.jsonOutput logic and the existing writeJsonEscapedString context) so the
same validation can be reused and frameCount/width/height handling remains
unchanged.
- Around line 501-509: Remove the redundant suppression for opts: delete the
line "(void)opts;" since opts is already referenced (opts.jsonOutput and
opts.schemaVersion) in the surrounding block; keep the unused-videoPath
suppression "(void)videoPath;" if needed and leave the calls to printJsonError
and the return statements unchanged (references: opts, videoPath,
printJsonError).
In `@tests/test_ccap_cli.cpp`:
- Around line 388-417: Extract a small reusable helper function (e.g.,
AssertJsonEnvelope or ExpectCliJsonEnvelope) and call it from
TEST_F(CCAPCLIDeviceTest, ListDevicesJson) and TEST_F(CCAPCLIDeviceTest,
ShowDeviceInfoJson) to centralize the repeated assertions for
"\"schema_version\":\"1.0\"", "\"command\":\"...\"", and "\"success\":..."; the
helper should accept the CLI output string and expected command name (or allow
command to be optional) and perform the common EXPECT_THAT checks, then replace
the duplicated checks in those tests (and the similar block around lines
753-768) with a single call to the new helper to reduce duplication and make
future schema changes easier to update.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b79b6b27-3776-4f3b-9388-d78b95e2cc76
📒 Files selected for processing (17)
AGENTS.mdREADME.mdREADME.zh-CN.mdcli/args_parser.cppcli/args_parser.hcli/ccap_cli.cppcli/ccap_cli_utils.cppcli/ccap_cli_utils.hdocs/content/cli.mddocs/content/cli.zh.mdskills/README.mdskills/ccap/PUBLISHING.mdskills/ccap/SKILL.mdskills/ccap/references/commands.mdskills/ccap/references/install.mdtests/test_ccap_cli.cpptests/test_cli_args_parser.cpp
| | `--json` | 对已支持的命令输出结构化 JSON | | ||
| | `--schema-version VERSION` | 设置 JSON 输出中的 schema_version 字段（默认：`1.0`） | |
There was a problem hiding this comment.
docs/ language policy violation in newly changed lines.
The updates at Line 88–Line 89 and Line 194–Line 210 are in Chinese, but this path is governed as English-only. Please move localized content out of docs/ or convert these updates to English in this location.
As per coding guidelines, .md files in docs/ must be English and require review before publishing.
Also applies to: 194-210
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/content/cli.zh.md` around lines 88 - 89, The changed lines in
docs/content/cli.zh.md introduce Chinese text for CLI flags (`--json` and
`--schema-version VERSION`) and other sections (lines 194–210) which violate the
English-only docs rule; update these lines to English equivalents (e.g.,
"`--json` | Output structured JSON for supported commands" and
"`--schema-version VERSION` | Set the schema_version field in JSON output
(default: `1.0`)") or move the Chinese content to a localized docs directory and
keep docs/content/cli.md strictly English; ensure references to the flags and
any headings use the same identifiers (`--json`, `--schema-version VERSION`) so
links/search remain valid.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cli/ccap_cli_utils.cpp`:
- Around line 491-498: The total_frames field currently does a
static_cast<int>(frameCount) which is undefined if frameCount is NaN/Inf; change
the output to use the same finite-number helper as frame_rate/duration_seconds
(call writeJsonFiniteNumberOrNull with frameCount) instead of casting, so
non-finite values become null; update the surrounding lines in
cli/ccap_cli_utils.cpp (the block that writes ",\"frame_rate\":",
writeJsonFiniteNumberOrNull, ",\"duration_seconds\":",
writeJsonFiniteNumberOrNull, and ",\"total_frames\":") to call
writeJsonFiniteNumberOrNull for frameCount (or conditionally cast only after
checking std::isfinite) and fix the inconsistent indentation on the lines that
write frame_rate/duration_seconds/total_frames to match the surrounding
formatting.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3b8a574f-6f3d-45fd-ad11-ad6cd85c5b8d
📒 Files selected for processing (8)
cli/args_parser.cppcli/ccap_cli.cppcli/ccap_cli_utils.cppcli/ccap_cli_utils.hdocs/content/cli.mddocs/content/cli.zh.mdskills/ccap/references/install.mdtests/test_cli_args_parser.cpp
✅ Files skipped from review due to trivial changes (3)
- tests/test_cli_args_parser.cpp
- skills/ccap/references/install.md
- docs/content/cli.md
🚧 Files skipped from review as they are similar to previous changes (3)
- cli/ccap_cli_utils.h
- cli/ccap_cli.cpp
- docs/content/cli.zh.md
|
@Auggie review |
| void writeJsonFiniteNumberOrNull(std::ostream& os, double value) { | ||
| if (std::isfinite(value)) { | ||
| os << value; | ||
| } else { | ||
| os << "null"; | ||
| } | ||
| } | ||
|
|
||
| void writeJsonFiniteIntOrNull(std::ostream& os, double value) { | ||
| if (std::isfinite(value) && value >= static_cast<double>(std::numeric_limits<int>::min()) | ||
| && value <= static_cast<double>(std::numeric_limits<int>::max())) { | ||
| os << static_cast<int>(value); | ||
| } else { | ||
| os << "null"; | ||
| } | ||
| } |
There was a problem hiding this comment.
JSON number formatting currently relies on the stream’s active locale and default precision (e.g., some locales use ',' as decimal separator), which can generate invalid JSON and/or lose precision for automation. Consider imbuing a classic/C locale on the JSON stream and using an explicit precision (e.g., max_digits10) when writing floating-point values.
| @@ -279,15 +490,49 @@ int printVideoInfo(const std::string& videoPath) { | |||
| int width = static_cast<int>(provider.get(ccap::PropertyName::Width)); | |||
| int height = static_cast<int>(provider.get(ccap::PropertyName::Height)); | |||
|
|
|||
There was a problem hiding this comment.
printVideoInfo assumes that provider.open(videoPath, false) opens a video file when CCAP_ENABLE_FILE_PLAYBACK is defined, but on platforms without file-mode support (notably Linux) this call routes to the camera open path and will fail (or could theoretically open a device whose name matches the path). After opening, verify provider.isFileMode() before reading Duration/FrameCount/etc, and if it’s false return a clear “file playback unsupported” JSON/text error (instead of “Failed to open video file”).
| ccap::Provider devProvider(deviceNames[index]); | ||
| std::optional<ccap::DeviceInfo> info; | ||
| if (devProvider.isOpened()) { | ||
| info = devProvider.getDeviceInfo(); | ||
| } | ||
|
|
||
| writeJsonDevice(os, index, deviceNames[index], info); |
There was a problem hiding this comment.
In --json mode, listDevices opens every camera (ccap::Provider devProvider(deviceName) opens the device in its constructor) to fetch capabilities. This can be slow and can trigger camera permission prompts just for enumeration, which is risky for automation. Consider emitting only index/name in --list-devices --json (or making capability probing opt-in via a separate flag) to keep the JSON listing lightweight and reliable.
| ccap::Provider devProvider(deviceNames[index]); | |
| std::optional<ccap::DeviceInfo> info; | |
| if (devProvider.isOpened()) { | |
| info = devProvider.getDeviceInfo(); | |
| } | |
| writeJsonDevice(os, index, deviceNames[index], info); | |
| // In JSON listing mode, avoid opening each device to probe capabilities. | |
| // Opening devices here can be slow and may trigger permission prompts, | |
| // which is undesirable for automation. We therefore emit only index/name | |
| // and pass no capability info. | |
| writeJsonDevice(os, index, deviceNames[index], std::nullopt); |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cli/ccap_cli_utils.cpp`:
- Around line 104-111: The helper writeJsonFiniteIntOrNull currently downcasts
to 32-bit int and returns "null" for values outside INT range or with fractional
parts; change it to accept and preserve full 64-bit frame counts by using
std::int64_t (or checking against
std::numeric_limits<std::int64_t>::min()/max()) and emitting the integer value
as a 64-bit integer when finite and within that range (or otherwise emit the
finite value directly as a number), and ensure the same logic is applied to the
text-mode "Total frames" branch so it no longer truncates or collapses large
frame counts to null/unknown; update references to static_cast<int>(...) to use
static_cast<std::int64_t>(...) or direct emission of the double as appropriate
in writeJsonFiniteIntOrNull and the Total frames code path.
In `@tests/test_ccap_cli.cpp`:
- Around line 388-395: The test TEST_F CCAPCLIDeviceTest::ListDevicesJson
currently asserts substrings in result.output; instead parse result.output as
JSON using the new JSON parser (the same parser added in this PR), then assert
the actual fields/types: check root is an object, "schema_version" equals "1.0",
"command" equals "list-devices", "success" is boolean true, and "devices" is an
array (optionally assert array elements have expected object fields). Update the
test to call runCLI("--list-devices --json"), parse result.output into a JSON
value, and replace EXPECT_THAT substring checks with precise assertions on the
parsed JSON; apply the same pattern to the other tests mentioned (lines 404-417,
753-769).
- Around line 404-417: The ShowDeviceInfoJson test currently accepts both
success and failure envelopes; update TEST_F(CCAPCLIDeviceTest,
ShowDeviceInfoJson) to require a true success path by asserting result.exitCode
== 0 (or failing the test if not) and only then check for "\"success\":true",
"\"device_count\":1", "\"supported_resolutions\":", and
"\"supported_pixel_formats\"" using runCLI("--device-info 0 --json"); then add a
new test (e.g., TEST_F(CCAPCLIDeviceTest, ShowDeviceInfoJson_ErrorPath)) that
calls runCLI with an invalid index/name (for example "--device-info 999 --json"
or an obviously invalid name), asserts a non-zero exitCode, and checks the JSON
error envelope contains "\"success\":false", "\"error\":", and "\"code\":".
Ensure tests reference the same runCLI helper and keep assertions strict for
each path.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: baf2bd69-e7c3-49ea-90e2-30f8488ffde2
📒 Files selected for processing (2)
cli/ccap_cli_utils.cpptests/test_ccap_cli.cpp
| void writeJsonFiniteIntOrNull(std::ostream& os, double value) { | ||
| if (std::isfinite(value) && value >= static_cast<double>(std::numeric_limits<int>::min()) | ||
| && value <= static_cast<double>(std::numeric_limits<int>::max())) { | ||
| os << static_cast<int>(value); | ||
| } else { | ||
| os << "null"; | ||
| } | ||
| } |
There was a problem hiding this comment.
Don't downcast valid frame counts to 32-bit int.
Both branches turn any finite value above INT_MAX into null/unknown, and they still silently truncate fractional values. That will misreport legitimate long-form recordings. Use a 64-bit integer check here, or emit the finite number directly.
🛠️ Possible fix
void writeJsonFiniteIntOrNull(std::ostream& os, double value) {
- if (std::isfinite(value) && value >= static_cast<double>(std::numeric_limits<int>::min())
- && value <= static_cast<double>(std::numeric_limits<int>::max())) {
- os << static_cast<int>(value);
+ if (std::isfinite(value) && std::trunc(value) == value
+ && value >= static_cast<double>(std::numeric_limits<long long>::min())
+ && value <= static_cast<double>(std::numeric_limits<long long>::max())) {
+ os << static_cast<long long>(value);
} else {
os << "null";
}
}And mirror the same wider/integral check in the text-mode Total frames branch.
Also applies to: 521-526
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cli/ccap_cli_utils.cpp` around lines 104 - 111, The helper
writeJsonFiniteIntOrNull currently downcasts to 32-bit int and returns "null"
for values outside INT range or with fractional parts; change it to accept and
preserve full 64-bit frame counts by using std::int64_t (or checking against
std::numeric_limits<std::int64_t>::min()/max()) and emitting the integer value
as a 64-bit integer when finite and within that range (or otherwise emit the
finite value directly as a number), and ensure the same logic is applied to the
text-mode "Total frames" branch so it no longer truncates or collapses large
frame counts to null/unknown; update references to static_cast<int>(...) to use
static_cast<std::int64_t>(...) or direct emission of the double as appropriate
in writeJsonFiniteIntOrNull and the Total frames code path.
| TEST_F(CCAPCLIDeviceTest, ListDevicesJson) { | ||
| auto result = runCLI("--list-devices --json"); | ||
| EXPECT_EQ(result.exitCode, 0); | ||
| EXPECT_THAT(result.output, ::testing::HasSubstr("\"schema_version\":\"1.0\"")); | ||
| EXPECT_THAT(result.output, ::testing::HasSubstr("\"command\":\"list-devices\"")); | ||
| EXPECT_THAT(result.output, ::testing::HasSubstr("\"success\":true")); | ||
| EXPECT_THAT(result.output, ::testing::HasSubstr("\"devices\":")); | ||
| } |
There was a problem hiding this comment.
Validate a JSON document, not just substrings.
Because Line 73 redirects stderr into result.output, these checks still pass if the CLI prints log noise or malformed JSON around the payload. Parse the full response with the parser added in this PR and assert field values/types so the JSON contract is actually enforced.
Also applies to: 404-417, 753-769
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/test_ccap_cli.cpp` around lines 388 - 395, The test TEST_F
CCAPCLIDeviceTest::ListDevicesJson currently asserts substrings in
result.output; instead parse result.output as JSON using the new JSON parser
(the same parser added in this PR), then assert the actual fields/types: check
root is an object, "schema_version" equals "1.0", "command" equals
"list-devices", "success" is boolean true, and "devices" is an array (optionally
assert array elements have expected object fields). Update the test to call
runCLI("--list-devices --json"), parse result.output into a JSON value, and
replace EXPECT_THAT substring checks with precise assertions on the parsed JSON;
apply the same pattern to the other tests mentioned (lines 404-417, 753-769).
| TEST_F(CCAPCLIDeviceTest, ShowDeviceInfoJson) { | ||
| auto result = runCLI("--device-info 0 --json"); | ||
| EXPECT_THAT(result.output, ::testing::HasSubstr("\"command\":\"device-info\"")); | ||
| if (result.exitCode == 0) { | ||
| EXPECT_THAT(result.output, ::testing::HasSubstr("\"success\":true")); | ||
| EXPECT_THAT(result.output, ::testing::HasSubstr("\"device_count\":1")); | ||
| EXPECT_THAT(result.output, ::testing::HasSubstr("\"supported_resolutions\":")); | ||
| EXPECT_THAT(result.output, ::testing::HasSubstr("\"supported_pixel_formats\":")); | ||
| } else { | ||
| EXPECT_THAT(result.output, ::testing::HasSubstr("\"success\":false")); | ||
| EXPECT_THAT(result.output, ::testing::HasSubstr("\"error\":")); | ||
| EXPECT_THAT(result.output, ::testing::HasSubstr("\"code\":")); | ||
| } | ||
| } |
There was a problem hiding this comment.
Make the success-path test fail on unexpected error envelopes.
This test goes green for both success:true and success:false, so a regression where --device-info 0 --json always fails would be missed. Keep this case strict on exitCode == 0, and cover the error path with a separate invalid-index/name test.
🧪 Tighten this test
TEST_F(CCAPCLIDeviceTest, ShowDeviceInfoJson) {
auto result = runCLI("--device-info 0 --json");
+ ASSERT_EQ(result.exitCode, 0) << result.output;
EXPECT_THAT(result.output, ::testing::HasSubstr("\"command\":\"device-info\""));
- if (result.exitCode == 0) {
- EXPECT_THAT(result.output, ::testing::HasSubstr("\"success\":true"));
- EXPECT_THAT(result.output, ::testing::HasSubstr("\"device_count\":1"));
- EXPECT_THAT(result.output, ::testing::HasSubstr("\"supported_resolutions\":"));
- EXPECT_THAT(result.output, ::testing::HasSubstr("\"supported_pixel_formats\":"));
- } else {
- EXPECT_THAT(result.output, ::testing::HasSubstr("\"success\":false"));
- EXPECT_THAT(result.output, ::testing::HasSubstr("\"error\":"));
- EXPECT_THAT(result.output, ::testing::HasSubstr("\"code\":"));
- }
+ EXPECT_THAT(result.output, ::testing::HasSubstr("\"success\":true"));
+ EXPECT_THAT(result.output, ::testing::HasSubstr("\"device_count\":1"));
+ EXPECT_THAT(result.output, ::testing::HasSubstr("\"supported_resolutions\":"));
+ EXPECT_THAT(result.output, ::testing::HasSubstr("\"supported_pixel_formats\":"));
}Then add a separate JSON error-path test that intentionally uses an invalid device index or name.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/test_ccap_cli.cpp` around lines 404 - 417, The ShowDeviceInfoJson test
currently accepts both success and failure envelopes; update
TEST_F(CCAPCLIDeviceTest, ShowDeviceInfoJson) to require a true success path by
asserting result.exitCode == 0 (or failing the test if not) and only then check
for "\"success\":true", "\"device_count\":1", "\"supported_resolutions\":", and
"\"supported_pixel_formats\"" using runCLI("--device-info 0 --json"); then add a
new test (e.g., TEST_F(CCAPCLIDeviceTest, ShowDeviceInfoJson_ErrorPath)) that
calls runCLI with an invalid index/name (for example "--device-info 999 --json"
or an obviously invalid name), asserts a non-zero exitCode, and checks the JSON
error envelope contains "\"success\":false", "\"error\":", and "\"code\":".
Ensure tests reference the same runCLI helper and keep assertions strict for
each path.
Summary
ccapCLI for device listing, device info, and video infoskills/ccap.githubas the distribution sourceVerification
./ccap_cli_args_parser_test --gtest_filter=CLIArgsParserTest.ParsesJsonOutputOptions./ccap_cli_test --gtest_filter=CCAPCLITest.VideoInfoJson./ccap_cli_test --gtest_filter=CCAPCLIDeviceTest.ListDevicesJson./ccap_cli_test --gtest_filter=CCAPCLIDeviceTest.ShowDeviceInfoJsonbash /Users/ergozi/work/git/CameraCapture/scripts/format_all.shNotes
skills/ccap/with a top-levelSKILL.mdAGENTS.mdremains as the repository discovery entry, while.github/skills/is no longer treated as the publish targetSummary by CodeRabbit
New Features
Bug Fix / Behavior
Documentation
Tests