feat(cli): add qtmesh scan — 3D asset pipeline linting - #272
Merged
Conversation
Add a new `scan` subcommand that recursively scans directories for 3D asset issues — like ESLint for 3D assets. Designed for CI integration with configurable rules, YAML config, and SARIF output. Key features: - YAML config (qtmesh.yml) with scan paths, rules, scopes, fix, report - 23 validation rules: format, size, complexity, naming, skeleton/bone, animation names/keyframes/duration, textures, materials - Scoped validation: different rules per folder (characters vs props) - Wildcard patterns for animation/bone name requirements - Min/max counterparts for all numeric limits - Output: text, JSON, SARIF 2.1.0 (GitHub Code Scanning compatible) - Auto-fix: file renaming for naming convention violations (--fix) - Exit codes for CI: --fail-on error|warning|info|never Also includes: - Documentation website page (docs.html) with full CLI reference - CI steps: native scan in unit-tests, Docker scan in docker-publish - Docker entrypoint and GitHub Action updated for scan command - 44 unit tests covering YAML parser, glob matching, all rules, scopes - Example config: qtmesh.example.yml Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds a new CLI subcommand Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as CLIPipeline::cmdScan
participant Config as ScanConfig
participant Engine as ScanEngine
participant Assimp as Assimp
participant Output as Formatter
CLI->>Config: loadFromFile() or defaults()
Config-->>CLI: ScanConfig
CLI->>Engine: run(config, scanRoot)
Engine->>Engine: enumerateFiles(config, scanRoot)
loop for each file
Engine->>Assimp: load asset file
Assimp-->>Engine: metadata or error
Engine->>Engine: inspectAsset()
Engine->>Engine: evaluateRules(asset, config)
Engine-->>Engine: findings
alt config.fixEnabled
Engine->>Engine: applyFixes(asset, findings)
end
end
Engine-->>CLI: ScanResult
alt --json
CLI->>Output: formatJson(result)
else --sarif
CLI->>Output: formatSarif(result)
else
CLI->>Output: formatText(result, config)
end
Output-->>CLI: formatted output
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 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 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: 10a78d6ec7
ℹ️ 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".
Comment on lines
+488
to
+489
| asset.relativePath = QDir(QFileInfo(oldPath).path()) | ||
| .relativeFilePath(newPath); |
There was a problem hiding this comment.
When a file_name_case fix renames a file, asset.relativePath is recomputed relative to the file's own parent directory, which drops intermediate folders (for example characters/PlayerModel.fbx becomes player_model.fbx). The formatters then correlate findings via f.file == asset.relativePath, so fixed files in subdirectories can lose their findings in text/JSON output and appear clean or collide with same-name files from other folders.
Useful? React with 👍 / 👎.
Comment on lines
+353
to
+357
| QVariantMap scopesMap = root.value("scopes").toMap(); | ||
| for (auto it = scopesMap.constBegin(); it != scopesMap.constEnd(); ++it) { | ||
| ScanScope scope; | ||
| scope.pathPattern = it.key(); | ||
| scope.rules = it.value().toMap(); |
There was a problem hiding this comment.
Scope precedence is documented as "later overrides earlier," but this loader converts scopes to QVariantMap and iterates it, which key-sorts entries rather than preserving declaration order. For overlapping scope globs, that can apply overrides in a different order than authored in qtmesh.yml, producing unexpected validation results.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
🧹 Nitpick comments (4)
src/main.cpp (1)
93-107: Add a breadcrumb for CLI command dispatch (includingscan).Line 106 hands off a user-facing CLI action to
CLIPipeline::run(...); addSentryReporter::addBreadcrumb(...)before dispatch so CLI command routing is traceable.As per coding guidelines:
**/*.cpp: Add Sentry breadcrumbs for all user-facing actions and significant operations using SentryReporter::addBreadcrumb() with categories: 'ui.action' for toolbar/menu clicks, 'ai.tool_call' for MCP tools, 'file.import'/'file.export' for I/O operations.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main.cpp` around lines 93 - 107, Before handing off to CLIPipeline::run(argc, argv), call SentryReporter::addBreadcrumb(...) to record the CLI command being dispatched (include the invoked subcommand such as "scan" and the raw argv contents or assembled command string in the breadcrumb message or data). Use the category "ui.action" per guidelines, set a concise message like "cli: dispatch <command>" and include any relevant data (e.g., command/subcommand and args) so the CLI routing is traceable; place this immediately above the CLIPipeline::run(...) call in main.cpp.src/ScanEngine_test.cpp (2)
606-747: Missing coverage for overlapping scopes.All current scope tests use non-overlapping patterns. Please add a case where two matching scopes set the same rule and the later declaration must win, since that ordering is part of
withScopeOverrides()'s contract.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ScanEngine_test.cpp` around lines 606 - 747, Add a unit test covering overlapping scopes so ordering semantics of withScopeOverrides() are validated: create a ScanConfig with two ScanScope entries whose pathPattern both match the same asset (e.g., "characters/**" and "characters/hero/**") and both set the same rule (e.g., "max_vertex_count" or "require_skeleton") to different values; call ScanConfig::withScopeOverrides("characters/hero.fbx") and assert that the value from the later/last-added scope wins (expected equals the second scope's setting), ensuring the test references ScanConfig::withScopeOverrides, ScanConfig::scopes and ScanScope::pathPattern to locate where to add the case.
541-600: Add the empty-animation / empty-skeleton regressions.These cases only assert name matching when an asset already has animations or bones. Please add cases where
require_animation_names/require_bone_namesare configured but the asset has none; that's where the current logic can silently miss invalid assets.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ScanEngine_test.cpp` around lines 541 - 600, Add tests for the empty-animation and empty-skeleton regressions: create new TESTs (e.g., EvaluateRules_RequireAnimationNames_Empty and EvaluateRules_RequireBoneNames_Empty) that build an AssetInfo with no animations (animationCount = 0 and animationNames empty) and an AssetInfo with no skeleton/bones (hasSkeleton = false or boneCount = 0 and boneNames empty), set ScanConfig::defaults() and populate config.requireAnimationNames and config.requireBoneNames with required patterns, call ScanEngine::evaluateRules(asset, config), and assert that findings contain the appropriate rule ("require_animation_names" or "require_bone_names") and that the finding messages report the missing required names/patterns (e.g., contain the configured names/patterns).src/ScanEngine.cpp (1)
502-553: Add breadcrumbs around this new scan flow.This new CLI path performs recursive reads and optional auto-fix I/O without any
SentryReporter::addBreadcrumb()calls, which makes scan failures and rename side effects harder to reconstruct.As per coding guidelines, "Add Sentry breadcrumbs for all user-facing actions and significant operations using SentryReporter::addBreadcrumb() with categories: 'ui.action' for toolbar/menu clicks, 'ai.tool_call' for MCP tools, 'file.import'/'file.export' for I/O operations".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ScanEngine.cpp` around lines 502 - 553, The run() flow in ScanEngine::run lacks Sentry breadcrumbs around major I/O and user-facing steps; add SentryReporter::addBreadcrumb() calls to record (1) scan start and each scanRoot before enumerateFiles (category "file.import"), (2) each file inspection before/after inspectAsset (category "file.import"), (3) any attempted modifications inside applyFixes and when a file is renamed or written (category "file.export"), and (4) when an asset.loadError or rule evaluation produces errors (category "file.import" or "ai.tool_call" as appropriate); include minimal payloads with keys like "scanRoot", "filePath", "action" ("inspect", "fix", "rename"), and error/message details so breadcrumb context can reconstruct failures.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/actions/qtmesh/action.yml:
- Around line 6-9: The action currently relies on shell-splitting of
inputs.options via read -r -a which preserves quotes and breaks quoted
arguments; change the action interface to accept a structured argument list
(e.g., a JSON array or newline-separated entries) instead of a single
shell-quoted string, update the action.yml input name/description to indicate
the new format (e.g., options-json or options-list) and modify the runner script
to parse it robustly (for JSON: use jq or fromJson to emit each element and pass
them as arguments; for newline-separated: use readarray -t args
<<<"$INPUT_OPTIONS" and then call qtmesh "${args[@]}"), and update docs/examples
to show the new format so quoted filenames and spaces are preserved.
In @.github/workflows/deploy.yml:
- Around line 1179-1204: The scan step "Scan repository assets with qtmesh scan"
never finds a QtMeshEditor binary because this job configures CMake with
-DBUILD_QT_MESH_EDITOR=OFF and additionally suppresses failures via the trailing
"|| { ... true; }"; fix by either enabling/building a CLI-capable target (turn
on BUILD_QT_MESH_EDITOR or add a small qtmesh CLI target) in this job so
TEST_BIN (./build/debug/QtMeshEditor, ./build/bin/QtMeshEditor,
./bin/QtMeshEditor) exists, or move the scan step into the job that produces the
QtMeshEditor/qtmesh artifact; also remove the "|| { ... true; }"
failure-suppression so that "$TEST_BIN --cli scan ..." returns a nonzero exit to
fail the job when scan errors occur.
In `@qtmesh.example.yml`:
- Around line 1-3: Update the example header link string
"https://github.com/nickvdp/QtMeshEditor" to point to the correct QtMesh
repository URL used by this project; locate the header comment lines containing
"qtmesh.yml — 3D asset pipeline linting configuration" and replace the incorrect
GitHub path with the project's actual repository URL so the example copy points
to the right project documentation.
In `@src/CLIPipeline.cpp`:
- Around line 1688-1718: The report writers currently always call
ScanEngine::formatJson and ignore ScanConfig::reportFormat; change the logic
that writes reportPath and config.reportOutput to consult config.reportFormat
(values "text", "json", "both") and call the appropriate serializer (e.g.,
ScanEngine::formatText for text, ScanEngine::formatJson for json); for "both"
emit both representations (write JSON and also the human-readable text form) —
for example write the primary output as requested and, when "both", also write
the text form to a sibling file (e.g., same path with ".txt" appended) so both
formats are produced; keep SARIF writing (ScanEngine::formatSarif and
sarifPath/config.sarifOutput) unchanged.
- Around line 1643-1646: After merging CLI values into config (when setting
config.failOn from failOn), validate the final config.failOn against the allowed
set (info, warning, error, never) — preferably case-insensitive — and if it is
not one of those values log a usage/error message and exit with status code 2;
update the same validation for the other merge point around lines 1721-1726.
Locate the assignments that set config.failOn and the exit-code logic
(references: variables failOn and config.failOn) and add a short validation
block that normalizes the string, checks membership in the allowed set, emits a
clear usage error, and returns/exit(2) on invalid input.
In `@src/ScanConfig.cpp`:
- Around line 352-359: The code builds scopes via root.value("scopes").toMap()
which yields a QVariantMap/QMap that iterates by sorted keys, breaking the
documented "later scopes override earlier ones" semantics; to fix, preserve
declaration order by reading the "scopes" object as a QVariant (or QJsonObject)
and iterate its keys in insertion order instead of using QVariantMap: replace
the use of QVariantMap scopesMap = root.value("scopes").toMap() and its iterator
with logic that obtains the original QVariant (e.g. QVariant scopesVar =
root.value("scopes")), access the underlying QJsonObject or QVariantList/keys in
insertion order, then construct ScanScope entries (setting scope.pathPattern,
scope.rules) and append them to config.scopes in the same order as declared so
withScopeOverrides/ScanScope and config.scopes reflect declaration precedence.
In `@src/ScanEngine.cpp`:
- Around line 480-483: The dry-run branch in ScanEngine.cpp is incorrectly
marking findings as applied: when config.dryRun is true the code appends the
preview to f.message and then sets f.fixed = true causing summaries/JSON/SARIF
to show the change as performed; instead, remove or avoid setting f.fixed in the
dry-run path (leave f.fixed unchanged/false) so the output only shows a preview
message (keep the existing f.message update referencing newName but do not set
f.fixed).
- Around line 531-545: The scan currently counts every finding into
result.errors/result.warnings/result.infos even when f.fixed is true, so scans
with applyFixes() still report failures; update the loop in ScanEngine.cpp (the
block iterating "for (const auto& f : findings)") to skip severity counting for
findings where f.fixed is true (still increment result.fixed for f.fixed), and
set hasError/hasWarning only when an unfixed finding has that severity; keep the
existing asset.loadError and passed/skipped logic unchanged so passed is set
only when there are no unfixed errors or warnings.
- Around line 484-489: The rename branch currently recomputes asset.relativePath
from the filesystem path (QFileInfo(oldPath).path()) which loses the original
scan-root-relative parent; instead keep the original asset.relativePath's
directory and only replace the basename with newName. After successful
QFile::rename(oldPath, newPath), set asset.filePath = newPath, set
asset.relativePath =
QDir(QFileInfo(asset.relativePath).path()).filePath(newName) (or equivalently
take dirname = QFileInfo(asset.relativePath).path() and join dirname with
newName), and then update f.message and f.fixed as before so reporters still
match findings by the preserved scan-root-relative path.
- Around line 428-455: The checks for config.requireAnimationNames and
config.requireBoneNames currently bail out when the asset has no animations
(asset.animationCount) or no skeleton (asset.hasSkeleton), allowing
empty/unanimated assets to skip name-only policies; remove those short-circuit
conditions so the loops always run: for require_animation_names iterate over
config.requireAnimationNames and treat each required name as "not found" unless
matchesWildcard finds it in asset.animationNames (findings.append as before,
keeping the existing message that will show an empty list when none exist), and
similarly for require_bone_names iterate over config.requireBoneNames and check
against asset.boneNames even if asset.hasSkeleton is false so missing bones are
reported via findings.append; keep existing identifiers
(config.requireAnimationNames, asset.animationCount, asset.animationNames,
config.requireBoneNames, asset.hasSkeleton, asset.boneNames, findings.append,
matchesWildcard) to locate the code.
In `@website/src/DocsApp.jsx`:
- Around line 599-600: The JSON example in the <CodeBlock> currently hard-codes
"2.23.0"; update it so the version is not stale by replacing the literal
"2.23.0" with a dynamic source (e.g., read from siteConfig.customFields.version
or a build ENV like process.env.VERSION) and fall back to a safe placeholder
like "2.x.x" if the dynamic value is missing; locate the <CodeBlock> in
DocsApp.jsx and substitute the string there so the rendered example always shows
the current or generic version.
In `@website/src/DocsApp.module.css`:
- Around line 231-233: The Stylelint rule flags the `composes: codeBlock;`
declaration in the `.yamlExample` CSS module; fix it by adding a local Stylelint
suppression comment immediately above the declaration (for example use /*
stylelint-disable-next-line value-keyword-case */) to allow the CSS Modules
`composes` pattern, or alternatively update the Stylelint config to whitelist
CSS Modules `composes` usage; target the `.yamlExample` selector and the
`composes` declaration when applying the suppression or config change.
---
Nitpick comments:
In `@src/main.cpp`:
- Around line 93-107: Before handing off to CLIPipeline::run(argc, argv), call
SentryReporter::addBreadcrumb(...) to record the CLI command being dispatched
(include the invoked subcommand such as "scan" and the raw argv contents or
assembled command string in the breadcrumb message or data). Use the category
"ui.action" per guidelines, set a concise message like "cli: dispatch <command>"
and include any relevant data (e.g., command/subcommand and args) so the CLI
routing is traceable; place this immediately above the CLIPipeline::run(...)
call in main.cpp.
In `@src/ScanEngine_test.cpp`:
- Around line 606-747: Add a unit test covering overlapping scopes so ordering
semantics of withScopeOverrides() are validated: create a ScanConfig with two
ScanScope entries whose pathPattern both match the same asset (e.g.,
"characters/**" and "characters/hero/**") and both set the same rule (e.g.,
"max_vertex_count" or "require_skeleton") to different values; call
ScanConfig::withScopeOverrides("characters/hero.fbx") and assert that the value
from the later/last-added scope wins (expected equals the second scope's
setting), ensuring the test references ScanConfig::withScopeOverrides,
ScanConfig::scopes and ScanScope::pathPattern to locate where to add the case.
- Around line 541-600: Add tests for the empty-animation and empty-skeleton
regressions: create new TESTs (e.g., EvaluateRules_RequireAnimationNames_Empty
and EvaluateRules_RequireBoneNames_Empty) that build an AssetInfo with no
animations (animationCount = 0 and animationNames empty) and an AssetInfo with
no skeleton/bones (hasSkeleton = false or boneCount = 0 and boneNames empty),
set ScanConfig::defaults() and populate config.requireAnimationNames and
config.requireBoneNames with required patterns, call
ScanEngine::evaluateRules(asset, config), and assert that findings contain the
appropriate rule ("require_animation_names" or "require_bone_names") and that
the finding messages report the missing required names/patterns (e.g., contain
the configured names/patterns).
In `@src/ScanEngine.cpp`:
- Around line 502-553: The run() flow in ScanEngine::run lacks Sentry
breadcrumbs around major I/O and user-facing steps; add
SentryReporter::addBreadcrumb() calls to record (1) scan start and each scanRoot
before enumerateFiles (category "file.import"), (2) each file inspection
before/after inspectAsset (category "file.import"), (3) any attempted
modifications inside applyFixes and when a file is renamed or written (category
"file.export"), and (4) when an asset.loadError or rule evaluation produces
errors (category "file.import" or "ai.tool_call" as appropriate); include
minimal payloads with keys like "scanRoot", "filePath", "action" ("inspect",
"fix", "rename"), and error/message details so breadcrumb context can
reconstruct failures.
🪄 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: 2cee41f1-dadf-462b-9d9a-bbb93efb8161
📒 Files selected for processing (21)
.github/actions/qtmesh/action.yml.github/workflows/deploy.ymlCLAUDE.mdCMakeLists.txtdocker-entrypoint.shqtmesh.example.ymlsrc/CLIPipeline.cppsrc/CLIPipeline.hsrc/CMakeLists.txtsrc/ScanConfig.cppsrc/ScanConfig.hsrc/ScanEngine.cppsrc/ScanEngine.hsrc/ScanEngine_test.cppsrc/main.cppwebsite/docs.htmlwebsite/src/DocsApp.jsxwebsite/src/DocsApp.module.csswebsite/src/data/content.jswebsite/src/docs-main.jsxwebsite/vite.config.js
Comment on lines
+6
to
+9
| description: 'Subcommand: info, fix, convert, anim, validate, lod, pose, scan' | ||
| required: true | ||
| input-file: | ||
| description: 'Input file path (relative to workspace)' | ||
| description: 'Input file or directory path (relative to workspace)' |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
INPUT_OPTIONS='--include "*.fbx,*.glb" --report "scan outputs/report.json"'
read -r -a opts <<< "$INPUT_OPTIONS"
printf 'arg[%d]=<%s>\n' 0 "${opts[0]}"
printf 'arg[%d]=<%s>\n' 1 "${opts[1]}"
printf 'arg[%d]=<%s>\n' 2 "${opts[2]}"
printf 'arg[%d]=<%s>\n' 3 "${opts[3]}"Repository: fernandotonon/QtMeshEditor
Length of output: 144
🏁 Script executed:
cat -n .github/actions/qtmesh/action.ymlRepository: fernandotonon/QtMeshEditor
Length of output: 2178
🏁 Script executed:
find . -type f \( -name "README*" -o -name "*.md" \) | head -20Repository: fernandotonon/QtMeshEditor
Length of output: 388
🏁 Script executed:
rg -i "scan" --type markdown -A 3 -B 1 | head -50Repository: fernandotonon/QtMeshEditor
Length of output: 2809
🏁 Script executed:
fd -type f -name "*.yml" -o -name "*.yaml" | xargs rg -l "qtmesh\|scan" | head -10Repository: fernandotonon/QtMeshEditor
Length of output: 240
Quoted scan options won't survive this action interface.
The new scan subcommand examples in the documentation (e.g., --include "*.fbx,*.glb", --report report.json) rely on quoted arguments, but the implementation tokenizes inputs.options with read -r -a at line 44. This approach preserves quote characters literally and splits on whitespace, breaking common invocations:
--include "*.fbx,*.glb"becomesarg[0]=--include,arg[1]="*.fbx,*.glb"(literal quotes passed to qtmesh)--report "path with spaces"splits at the space, truncating the path
Use a structured input format (JSON, newline-separated args, or positional parameters) instead of shell-split strings.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/actions/qtmesh/action.yml around lines 6 - 9, The action currently
relies on shell-splitting of inputs.options via read -r -a which preserves
quotes and breaks quoted arguments; change the action interface to accept a
structured argument list (e.g., a JSON array or newline-separated entries)
instead of a single shell-quoted string, update the action.yml input
name/description to indicate the new format (e.g., options-json or options-list)
and modify the runner script to parse it robustly (for JSON: use jq or fromJson
to emit each element and pass them as arguments; for newline-separated: use
readarray -t args <<<"$INPUT_OPTIONS" and then call qtmesh "${args[@]}"), and
update docs/examples to show the new format so quoted filenames and spaces are
preserved.
Comment on lines
+1179
to
+1204
| - name: Scan repository assets with qtmesh scan | ||
| if: success() | ||
| env: | ||
| DISPLAY: :99 | ||
| QT_QPA_PLATFORM: xcb | ||
| LIBGL_ALWAYS_SOFTWARE: 1 | ||
| MESA_GL_VERSION_OVERRIDE: "3.3" | ||
| run: | | ||
| TEST_BIN="" | ||
| if [ -f "./build/debug/QtMeshEditor" ]; then | ||
| TEST_BIN="./build/debug/QtMeshEditor" | ||
| elif [ -f "./build/bin/QtMeshEditor" ]; then | ||
| TEST_BIN="./build/bin/QtMeshEditor" | ||
| elif [ -f "./bin/QtMeshEditor" ]; then | ||
| TEST_BIN="./bin/QtMeshEditor" | ||
| fi | ||
| if [ -z "$TEST_BIN" ]; then | ||
| echo "No QtMeshEditor binary found, skipping scan" | ||
| exit 0 | ||
| fi | ||
| echo "Scanning repository media/ assets..." | ||
| "$TEST_BIN" --cli scan media/ --include "*.mesh" --fail-on error || { | ||
| echo "::warning::Asset scan found errors in media/" | ||
| # Don't fail the build — informational for now | ||
| true | ||
| } |
There was a problem hiding this comment.
This scan step never actually gates unit-tests-linux.
This job configures CMake with -DBUILD_QT_MESH_EDITOR=OFF, so QtMeshEditor is not produced here and the step falls into the “skipping scan” branch on every run. Even if that changes later, the trailing || { ... true; } still suppresses a failing scan, so asset errors can never fail this job. Build a CLI-capable target in this job, or run the scan in a job that produces qtmesh/QtMeshEditor.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/deploy.yml around lines 1179 - 1204, The scan step "Scan
repository assets with qtmesh scan" never finds a QtMeshEditor binary because
this job configures CMake with -DBUILD_QT_MESH_EDITOR=OFF and additionally
suppresses failures via the trailing "|| { ... true; }"; fix by either
enabling/building a CLI-capable target (turn on BUILD_QT_MESH_EDITOR or add a
small qtmesh CLI target) in this job so TEST_BIN (./build/debug/QtMeshEditor,
./build/bin/QtMeshEditor, ./bin/QtMeshEditor) exists, or move the scan step into
the job that produces the QtMeshEditor/qtmesh artifact; also remove the "|| {
... true; }" failure-suppression so that "$TEST_BIN --cli scan ..." returns a
nonzero exit to fail the job when scan errors occur.
Comment on lines
+509
to
+549
| QStringList roots; | ||
| if (!rootOverride.isEmpty()) { | ||
| roots.append(QDir(rootOverride).absolutePath()); | ||
| } else if (!config.roots.isEmpty()) { | ||
| for (const auto& r : config.roots) | ||
| roots.append(QDir(r).absolutePath()); | ||
| } else { | ||
| roots.append(QDir::currentPath()); | ||
| } | ||
|
|
||
| // Enumerate and inspect all files across all roots | ||
| for (const auto& scanRoot : roots) { | ||
| QStringList files = enumerateFiles(config, scanRoot); | ||
|
|
||
| for (const auto& filePath : files) { | ||
| AssetInfo asset = inspectAsset(filePath, scanRoot); | ||
| QList<Finding> findings = evaluateRules(asset, config); | ||
|
|
||
| // Apply fixes where possible | ||
| applyFixes(config, asset, findings); | ||
|
|
||
| // Tally | ||
| bool hasError = false, hasWarning = false; | ||
| for (const auto& f : findings) { | ||
| switch (f.severity) { | ||
| case Severity::Error: result.errors++; hasError = true; break; | ||
| case Severity::Warning: result.warnings++; hasWarning = true; break; | ||
| case Severity::Info: result.infos++; break; | ||
| } | ||
| if (f.fixed) result.fixed++; | ||
| } | ||
|
|
||
| if (asset.loadError) | ||
| result.skipped++; | ||
| else if (!hasError && !hasWarning) | ||
| result.passed++; | ||
|
|
||
| result.scanned++; | ||
| result.findings.append(findings); | ||
| result.assets.append(asset); | ||
| } |
There was a problem hiding this comment.
Findings are ambiguous when more than one root is scanned.
inspectAsset(filePath, scanRoot) stores relativePath per root, but the result model and formatters key everything on that string alone. Two different roots containing the same relative path will have findings merged onto the wrong asset.
Comment on lines
+231
to
+233
| .yamlExample { | ||
| composes: codeBlock; | ||
| border-left: 3px solid var(--accent-2); |
There was a problem hiding this comment.
Stylelint failure on composes value casing at Line 232.
The current rule set flags composes: codeBlock;, which can fail lint/CI for this file. Add a local suppression (or adjust stylelint config) for this CSS Modules pattern.
🔧 Minimal fix
.yamlExample {
+ /* stylelint-disable-next-line value-keyword-case */
composes: codeBlock;
border-left: 3px solid var(--accent-2);
}📝 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.
Suggested change
| .yamlExample { | |
| composes: codeBlock; | |
| border-left: 3px solid var(--accent-2); | |
| .yamlExample { | |
| /* stylelint-disable-next-line value-keyword-case */ | |
| composes: codeBlock; | |
| border-left: 3px solid var(--accent-2); |
🧰 Tools
🪛 Stylelint (17.6.0)
[error] 232-232: Expected "codeBlock" to be "codeblock" (value-keyword-case)
(value-keyword-case)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@website/src/DocsApp.module.css` around lines 231 - 233, The Stylelint rule
flags the `composes: codeBlock;` declaration in the `.yamlExample` CSS module;
fix it by adding a local Stylelint suppression comment immediately above the
declaration (for example use /* stylelint-disable-next-line value-keyword-case
*/) to allow the CSS Modules `composes` pattern, or alternatively update the
Stylelint config to whitelist CSS Modules `composes` usage; target the
`.yamlExample` selector and the `composes` declaration when applying the
suppression or config change.
The tests/ directory has a separate file list for standalone test executables (MaterialEditorQML_*_test). These link CLIPipeline.cpp which now calls ScanEngine functions, so they need the scan sources. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Adds a dedicated CI job that scans repo assets using both the Docker image (ghcr.io/fernandotonon/qtmesh:latest) and the composite GitHub Action. Both steps use continue-on-error since the latest published image may not have the scan command until this PR is released. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/CMakeLists.txt (1)
74-75: Reduce source-list drift by sharing core sources via a common target.This addition fixes the immediate test-linking gap, but manually mirroring
src/CMakeLists.txtinTEST_SRC_FILESis brittle. Consider moving shared app/test sources (including scan files) into a library target and linking both app and tests to it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/CMakeLists.txt` around lines 74 - 75, Tests currently duplicate core source files (e.g., ScanConfig.cpp, ScanEngine.cpp) in TEST_SRC_FILES which causes drift; instead, create a shared library target (e.g., add_library(core_sources STATIC ...) containing the core sources referenced in src/CMakeLists.txt) and replace direct file lists in TEST_SRC_FILES by linking both the application and test targets to that library via target_link_libraries(app_target PRIVATE core_sources) and target_link_libraries(test_target PRIVATE core_sources); update tests/CMakeLists.txt to remove the duplicated filenames and link the test target to the new core_sources target.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/CMakeLists.txt`:
- Around line 74-75: Tests currently duplicate core source files (e.g.,
ScanConfig.cpp, ScanEngine.cpp) in TEST_SRC_FILES which causes drift; instead,
create a shared library target (e.g., add_library(core_sources STATIC ...)
containing the core sources referenced in src/CMakeLists.txt) and replace direct
file lists in TEST_SRC_FILES by linking both the application and test targets to
that library via target_link_libraries(app_target PRIVATE core_sources) and
target_link_libraries(test_target PRIVATE core_sources); update
tests/CMakeLists.txt to remove the duplicated filenames and link the test target
to the new core_sources target.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4747169f-89ee-48d3-a3f1-500445c6e024
📒 Files selected for processing (2)
.github/workflows/deploy.ymltests/CMakeLists.txt
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/deploy.yml
Bugs fixed: - dry-run no longer marks findings as f.fixed (was inflating "fixed" count) - require_animation_names/require_bone_names no longer bail out on empty assets — missing animations/bones are now reported even when the asset has no skeleton or animations at all - File rename in applyFixes preserves scan-root-relative directory path - Fixed findings no longer counted toward error/warning totals - Scope declaration order preserved via _order key in YAML parser (QVariantMap sorts alphabetically, breaking "later overrides earlier") - --fail-on validated against allowed values (info/warning/error/never) - Report file format now respects config.reportFormat (was always JSON) - Fixed wrong GitHub URL in qtmesh.example.yml - Replaced hardcoded version "2.23.0" with "2.x.x" in docs Improvements: - Sentry breadcrumbs added to ScanEngine::run() and applyFixes() - 3 new tests: empty-asset regressions, overlapping-scopes ordering Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Validates the 4 loadable demo models in media/ (excludes robot.mesh which uses an unsupported Ogre mesh version). CI steps now use the config file instead of inline flags. Rules: max 50K verts, 50MB, 16 meshes/materials, 500 keyframes, 30s duration. All demo assets require skeleton + animations. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
This was referenced Apr 10, 2026
This was referenced May 12, 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
qtmesh scancommand: recursively scan directories for 3D asset issues, like ESLint for 3D modelsqtmesh.yml: different rules per folder (e.g.characters/**requires skeleton + specific animations/bones,props/**has lower vertex limits)--fixrenames files violating naming conventions;--dry-runfor previewdocs.htmlpage with full CLI reference for all 8 commands, scan config schema, rules reference, CI/CD patternsqtmesh.example.yml), version bump to 2.24.0CLI Examples
Example Config (qtmesh.yml)
Test plan
qtmesh scan media/with various configs (text, JSON, SARIF output)🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation
Tests
Chores