Improve scan CLI UX, add scan rule override flags, and update docs - #280
Merged
Conversation
…-80-again # Conflicts: # src/CLIPipeline_test.cpp # src/ScanEngine_test.cpp
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant CLI as CLI
participant Config as ConfigLoader
participant Engine as ScanEngine
participant Reporter as Reporter
User->>CLI: run `qtmesh scan` with flags
CLI->>Config: load qtmesh.yml / qtmesh.json (if present)
CLI->>CLI: parse flags (accept --opt value & --opt=value, booleans/negations, validate)
CLI->>Config: inject CLI overrides as final ScanScope (CLI precedence)
Config->>Engine: invoke scan with effective ScanConfig
Engine->>Reporter: produce structured scan results
Reporter->>CLI: format output (apply icon colors only if colorize enabled)
CLI->>User: print terminal summary / write JSON/SARIF reports
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 64e0311d92
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/CLIPipeline_test.cpp (2)
1179-1184: This regression check is too permissive now.Accepting
1here means the test no longer protects the intended zero-animation success path. Please switch to a deterministic fixture/import path and keep asserting the success case explicitly.Based on learnings,
CLIPipeline::cmdAnimlist mode should return success and emitNo animations found./[]when a skeleton exists but has zero animations.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/CLIPipeline_test.cpp` around lines 1179 - 1184, The test currently allows textRc == 1 which masks failures; change the test to use a deterministic fixture/import path that guarantees the file is loaded (a model with a skeleton but zero animations) and assert success explicitly: replace the loose EXPECT_TRUE(textRc == 0 || textRc == 1) with an ASSERT_EQ(textRc, 0) (and keep ASSERT_EQ(textRc, jsonRc)); additionally verify CLIPipeline::cmdAnim list mode output contains the expected no-animation indicators (e.g. "No animations found." and "[]") so the test fails if the loader or list-mode handling regresses.
2134-2187: These scan override tests should isolate auto-loaded config.
cmdScan()auto-detectsqtmesh.yml/qtmesh.yaml/qtmesh.jsonwhen--configis omitted, so these cases can pick up unrelated repo-local rules and change the expected exit code. Wrap them inScopedCurrentDir(tmpDir.path())or pass a minimal temp config so the failure comes only from the override under test.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/CLIPipeline_test.cpp` around lines 2134 - 2187, The tests calling CLIPipeline::cmdScan are inheriting repo-local qtmesh.yml/qtmesh.yaml/qtmesh.json because they omit --config, causing flaky exit codes; fix each test (e.g., the tests using TestArgv and invoking CLIPipeline::cmdScan) by isolating auto-loaded config either by wrapping the test body with ScopedCurrentDir(tmpDir.path()) or by creating a minimal temp config file in tmpDir and passing its path via "--config" in TestArgv so only the specific override under test (e.g., --max-vertices, --max-file-size-mb, --allowed-formats) drives the failure.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/CLIPipeline.cpp`:
- Around line 1986-2061: The CLI currently treats empty string/list flags as
"not provided" by checking QString::isEmpty(), so you must adopt the same
sentinel approach used for other overrides (like maxFileSizeMbOverride checks)
for forbiddenExtensionsArg, fileNameCaseOverride, requiredAnimationNamesArg, and
requiredBoneNamesArg: when parsing the CLI set a distinct sentinel value (or a
parallel boolean "provided" flag) when the flag is present even if empty, then
change the checks in the block that applies overrides to test that sentinel/flag
instead of isEmpty(), apply the empty value to config.forbiddenExtensions,
config.fileNameCase, config.requireAnimationNames, and config.requireBoneNames
when the sentinel indicates an explicit empty override, and ensure those entries
are added to cliRuleOverrides (keys "forbidden_extensions", "file_name_case",
"require_animation_names", "require_bone_names") when the sentinel/flag is set
so scoped rules can be cleared from the CLI.
In `@website/src/DocsApp.jsx`:
- Around line 406-409: The paragraph in DocsApp.jsx that mentions loading
qtmesh.yml/qtmesh.json underspecifies supported filenames; update the string
inside the <p className={s.para}> (the paragraph that currently references
<Code>qtmesh.yml</Code>/<Code>qtmesh.json</Code>) to also include
<Code>qtmesh.yaml</Code> so the note reads that CLI overrides are applied after
loading qtmesh.yml/qtmesh.yaml/qtmesh.json (or similar wording) to reflect the
runtime auto-detection.
---
Nitpick comments:
In `@src/CLIPipeline_test.cpp`:
- Around line 1179-1184: The test currently allows textRc == 1 which masks
failures; change the test to use a deterministic fixture/import path that
guarantees the file is loaded (a model with a skeleton but zero animations) and
assert success explicitly: replace the loose EXPECT_TRUE(textRc == 0 || textRc
== 1) with an ASSERT_EQ(textRc, 0) (and keep ASSERT_EQ(textRc, jsonRc));
additionally verify CLIPipeline::cmdAnim list mode output contains the expected
no-animation indicators (e.g. "No animations found." and "[]") so the test fails
if the loader or list-mode handling regresses.
- Around line 2134-2187: The tests calling CLIPipeline::cmdScan are inheriting
repo-local qtmesh.yml/qtmesh.yaml/qtmesh.json because they omit --config,
causing flaky exit codes; fix each test (e.g., the tests using TestArgv and
invoking CLIPipeline::cmdScan) by isolating auto-loaded config either by
wrapping the test body with ScopedCurrentDir(tmpDir.path()) or by creating a
minimal temp config file in tmpDir and passing its path via "--config" in
TestArgv so only the specific override under test (e.g., --max-vertices,
--max-file-size-mb, --allowed-formats) drives the failure.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: ab8d8f7a-86b9-4048-abe6-502f1a5f8ddb
📒 Files selected for processing (3)
src/CLIPipeline.cppsrc/CLIPipeline_test.cppwebsite/src/DocsApp.jsx
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/CLIPipeline_test.cpp (1)
2134-2289: Optional: extract shared scan-fixture setup helper to reduce duplication.The repeated temp-dir + assets + minimal OBJ setup across these tests could be wrapped in a small helper for maintainability.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/CLIPipeline_test.cpp` around lines 2134 - 2289, Many tests (e.g., TESTs MaxVerticesOverrideReturnsFailure, MaxVerticesOverrideWithEqualsReturnsFailure, MaxFileSizeOverrideReturnsFailure, AllowedFormatsOverrideReturnsFailure, RequireSkeletonAndAnimationsCanBeDisabledFromCli, CliOverridesTakePrecedenceOverScopedRules, EmptyCliOverridesCanClearScopedRules) repeat the same temp-dir + assets creation + writeMinimalObj setup; extract that into a small helper (e.g., SetupScanTestAssets or a test fixture class like CLIPipelineScanTest with a method setupAssets) that returns the rootPath (and optionally config path and ScopedCurrentDir) so each TEST calls the helper and then only prepares args and assertions, replacing the duplicated QTemporaryDir, ScopedCurrentDir, mkdir, and writeMinimalObj blocks with a single call to the helper.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@website/src/DocsApp.jsx`:
- Around line 421-425: The hardcoded summary line in DocsApp.jsx is inconsistent
with the sample findings (shows "✗ Errors: 4" while actual errors are 3); update
the rendering in the DocsApp component so the summary numbers are computed from
the same data source used for the example findings (e.g., derive
scanned/passed/warnings/errors from the findings array or the findingsSummary
object rather than hardcoding "4"), or change the hardcoded "Errors" value to
match the sample (3) so the CodeBlock output (the string that currently contains
"• Scanned: 4 ✓ Passed: 1 ▲ Warnings: 1 ✗ Errors: 4 ⏱ Time: 0.3s") is consistent
with the example details.
---
Nitpick comments:
In `@src/CLIPipeline_test.cpp`:
- Around line 2134-2289: Many tests (e.g., TESTs
MaxVerticesOverrideReturnsFailure, MaxVerticesOverrideWithEqualsReturnsFailure,
MaxFileSizeOverrideReturnsFailure, AllowedFormatsOverrideReturnsFailure,
RequireSkeletonAndAnimationsCanBeDisabledFromCli,
CliOverridesTakePrecedenceOverScopedRules, EmptyCliOverridesCanClearScopedRules)
repeat the same temp-dir + assets creation + writeMinimalObj setup; extract that
into a small helper (e.g., SetupScanTestAssets or a test fixture class like
CLIPipelineScanTest with a method setupAssets) that returns the rootPath (and
optionally config path and ScopedCurrentDir) so each TEST calls the helper and
then only prepares args and assertions, replacing the duplicated QTemporaryDir,
ScopedCurrentDir, mkdir, and writeMinimalObj blocks with a single call to the
helper.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1bf9bf67-fc5e-4872-a2b2-e2a21e1b57f1
📒 Files selected for processing (3)
src/CLIPipeline.cppsrc/CLIPipeline_test.cppwebsite/src/DocsApp.jsx
✅ Files skipped from review due to trivial changes (1)
- src/CLIPipeline.cpp
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 `@src/CLIPipeline_test.cpp`:
- Line 1180: The test assertion expecting failure is inverted for skeletons with
zero animations: update the CLIPipeline_test.cpp assertion(s) that check
CLIPipeline::cmdAnim in list mode (the ASSERT_EQ(textRc, 1) line) to expect
success (0) instead of error (1); ensure any corresponding json-mode
assertion(s) are also changed to ASSERT_EQ(..., 0) so the test matches
CLIPipeline::cmdAnim behavior (returns 0 and emits "No animations found." or
"[]").
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: e415b873-2b8c-46eb-9db5-98aca3522cff
📒 Files selected for processing (2)
src/CLIPipeline_test.cppwebsite/src/DocsApp.jsx
✅ Files skipped from review due to trivial changes (1)
- website/src/DocsApp.jsx
|
This was referenced Apr 11, 2026
6 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR bundles the requested cleanup, coverage improvements, and scan CLI UX/documentation updates.
Core code improvements
PrimitivesWidgetcleanupMCPServerdeduplication/refactorTransformCommandstestsScan CLI UX improvements
OKgreenWARNyellowERRORred--max-verticesCLI override (both--max-vertices 1000and--max-vertices=1000).--allowed-formats,--forbidden-extensions--max-file-size-mb,--min-file-size-mb--max-meshes,--min-meshes--max-materials,--min-materials--max-vertices,--min-vertices--require-skeleton/--no-require-skeleton--require-animations/--no-require-animations--allow-embedded-textures/--disallow-embedded-textures--require-textures-exist/--no-require-textures-exist--allow-missing-materials/--disallow-missing-materials--file-name-case--max-anim-keyframes,--min-anim-keyframes--max-anim-duration,--min-anim-duration--require-animation-names,--require-bone-namesWebsite/docs updates
website/src/DocsApp.jsx) to include the new scan override flags and examples.Validation
QT_QPA_PLATFORM=offscreen build_cov_local/debug/UnitTests --gtest_filter='CLIPipelineCmdScan*'QT_QPA_PLATFORM=offscreen build_cov_local/debug/UnitTests --gtest_filter='ScanEngineTest.Run_*:CLIPipelineCmdScan*'cd website && npm run buildbuild_test/bin/qtmesh scan ...for streaming output and CLI override behavior.Notes
skippedcurrently tracks load/parse failures (loadError) and those also contribute toerrorsviaload_errorfindings.Summary by CodeRabbit
New Features
--flag valueand--flag=value, boolean negation, and empty flags to clear scoped rules; CLI overrides take precedence.Documentation
Tests
Bug Fixes