feat(cli): scan token-first rules, default path and include globs - #289
Conversation
- Default scan path to current directory; GitHub Action input-file defaults to '.' - When ingest token is set and --config is omitted, fetch rules from QtMesh Cloud before auto-detected local qtmesh.yml|yaml|json - Default scan.include to Assimp-registered extensions plus Ogre .mesh/.mesh.xml - Tests: clear QTMESH_* env for local-yml case; assert default include globs - Bump version to 2.27.0 Made-with: Cursor
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 47 minutes and 24 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThe PR bumps project version to 2.27.0, makes the GitHub Action Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI (cmdScan)
participant Token as Token Resolver
participant Cloud as QtMeshCloudClient
participant Local as Local Config Loader
participant Defaults as ScanConfig Defaults
CLI->>Token: resolveIngestToken()
alt Token present
Token-->>CLI: token
CLI->>Cloud: fetchRules(token)
alt Cloud success
Cloud-->>CLI: remote rules
else Cloud failure
Cloud-->>CLI: error/null
CLI->>Defaults: use defaults()
Defaults-->>CLI: default rules
end
else No token
Token-->>CLI: empty/null
CLI->>Local: load ./qtmesh.yml
alt Local config found
Local-->>CLI: local rules
else not found
Local-->>CLI: not found
CLI->>Defaults: use defaults()
Defaults-->>CLI: default rules
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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: 86509a5616
ℹ️ 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".
| } | ||
| // Ogre mesh formats used by the editor (may or may not appear as separate Assimp importers) | ||
| extSet.insert(QStringLiteral("mesh")); | ||
| extSet.insert(QStringLiteral("mesh.xml")); |
There was a problem hiding this comment.
Adding mesh.xml to the default include set means these assets are now scanned by default, but rule evaluation still keys off asset.format from ScanEngine::inspectAsset() (QFileInfo::suffix()), which resolves foo.mesh.xml to xml instead of mesh.xml. In projects that use allowed_formats/forbidden_extensions, this misclassifies Ogre mesh XML files and can produce incorrect pass/fail results (for example, allowed_formats: [mesh.xml] will wrongly reject them unless xml is also allowed).
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: 1
🧹 Nitpick comments (2)
src/ScanConfig.cpp (2)
291-322: Consider caching the computed default patterns.
defaultIncludePatternsForAssimpImports()creates anAssimp::Importerand iterates all registered importers on every invocation. Since Assimp's registered importers are static for a given build, the result is deterministic. This function is called:
- In the default constructor (line 287)
- Twice in
fromVariantMapfallback paths (lines 366, 372)If
ScanConfigobjects are created frequently, consider caching the result:♻️ Optional: Cache the computed patterns
QStringList ScanConfig::defaultIncludePatternsForAssimpImports() { + static const QStringList cached = []() { QSet<QString> extSet; Assimp::Importer importer; // ... existing logic ... globs.sort(Qt::CaseInsensitive); return globs; + }(); + return cached; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ScanConfig.cpp` around lines 291 - 322, defaultIncludePatternsForAssimpImports() recreates Assimp::Importer and recomputes the same QStringList on every call; make the computed patterns persistent by computing them once and returning the cached result on subsequent calls (e.g., use a function-local static QStringList cachedPatterns computed on first entry inside ScanConfig::defaultIncludePatternsForAssimpImports), preserving the existing logic (including inserting "mesh" and "mesh.xml"), then return cachedPatterns (local static initialization is thread-safe in C++11+).
363-372: Redundant fallback logic and unclear empty-list semantics.Two observations:
Redundant check: Lines 371-372 appear redundant. If
scan.includeis present, lines 365-366 already restore defaults when empty. Ifscan.includeis absent, the constructor (line 287) already initializedincludePatternswith defaults. The only path to an emptyincludePatternsat line 371 would be if the constructor didn't run, which isn't possible.User intent ambiguity: If a user explicitly sets
scan.include: []in their config, they might intend "scan no files." The current logic overrides this to defaults, which may be unexpected. Consider whether this is intentional behavior.♻️ Simplify the fallback logic
If the intent is "always have defaults when config doesn't specify includes":
if (scan.contains("include")) { config.includePatterns = scan.value("include").toStringList(); - if (config.includePatterns.isEmpty()) - config.includePatterns = ScanConfig::defaultIncludePatternsForAssimpImports(); } - if (scan.contains("exclude")) - config.excludePatterns = scan.value("exclude").toStringList(); - } - if (config.includePatterns.isEmpty()) - config.includePatterns = ScanConfig::defaultIncludePatternsForAssimpImports(); + if (scan.contains("exclude")) + config.excludePatterns = scan.value("exclude").toStringList();The constructor already provides defaults; only override if the user explicitly provides a non-empty list.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ScanConfig.cpp` around lines 363 - 372, The fallback logic is redundant and overrides an explicit empty user list; update the parsing in ScanConfig so the constructor's defaults (set in the ScanConfig constructor) remain the only automatic fallback: in the block handling scan.contains("include") assign config.includePatterns = scan.value("include").toStringList() but do not replace an explicit empty list with ScanConfig::defaultIncludePatternsForAssimpImports(); then remove the later unconditional if (config.includePatterns.isEmpty()) fallback. This preserves constructor defaults when the key is absent but respects an explicit empty include list from the user.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@action.yml`:
- Around line 13-16: The default for the input-file input now applies to all
commands and causes non-scan subcommands to receive '.'; change runtime handling
so the default '.' is only used when command == "scan" and all other commands
must explicitly validate that input-file is provided and is not '.'; update the
container entrypoint or CLI bootstrap to check the action input named input-file
and the command string (e.g., "scan", "info", "convert", "fix", "anim",
"validate", "lod", "pose") and: if command == "scan" allow default '.' to be
substituted, otherwise reject '.' (or missing) with a clear error and exit
non-zero so non-scan commands fail fast. Ensure the validation mirrors
CLIPipeline.cpp behavior (reject non-directory targets for scan) and keep
action.yml's input-file description unchanged except for runtime gating.
---
Nitpick comments:
In `@src/ScanConfig.cpp`:
- Around line 291-322: defaultIncludePatternsForAssimpImports() recreates
Assimp::Importer and recomputes the same QStringList on every call; make the
computed patterns persistent by computing them once and returning the cached
result on subsequent calls (e.g., use a function-local static QStringList
cachedPatterns computed on first entry inside
ScanConfig::defaultIncludePatternsForAssimpImports), preserving the existing
logic (including inserting "mesh" and "mesh.xml"), then return cachedPatterns
(local static initialization is thread-safe in C++11+).
- Around line 363-372: The fallback logic is redundant and overrides an explicit
empty user list; update the parsing in ScanConfig so the constructor's defaults
(set in the ScanConfig constructor) remain the only automatic fallback: in the
block handling scan.contains("include") assign config.includePatterns =
scan.value("include").toStringList() but do not replace an explicit empty list
with ScanConfig::defaultIncludePatternsForAssimpImports(); then remove the later
unconditional if (config.includePatterns.isEmpty()) fallback. This preserves
constructor defaults when the key is absent but respects an explicit empty
include list from the user.
🪄 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: 8306ff3e-3321-4bf5-b494-9869f02f8896
📒 Files selected for processing (8)
.github/actions/qtmesh/action.ymlCMakeLists.txtaction.ymlsrc/CLIPipeline.cppsrc/CLIPipeline_test.cppsrc/ScanConfig.cppsrc/ScanConfig.hsrc/ScanEngine_test.cpp
- Fix build failures: aiImporterDesc incomplete type on Linux/macOS/Windows by including assimp/importerdesc.h - Cache default include glob computation (Assimp importer enumeration) - Respect explicit empty scan.include (treat as configured override) - GitHub Action: require input-file for non-scan commands; scan still defaults to '.' Made-with: Cursor
Made-with: Cursor
Made-with: Cursor
…precedence-and-default-globs
Update website docs and README links from qtmesh.ftonon.uk to qtmesh.dev (including API host). Made-with: Cursor
|
1 participant
Summary
qtmesh scanwith no path scans the current directory (same as ScanEngine when no override and noscan.rootsin config).--configis not passed: ingest token → fetch rules from QtMesh Cloud → on failure use built-in defaults; without a token, auto-loadqtmesh.yml/qtmesh.yaml/qtmesh.jsonif present, else defaults.scan.include: all file extensions registered by Assimp importers, plus.meshand.mesh.xmlfor Ogre; overridden only when set in YAML/JSON/API config.input-fileis optional, default..Testing
qtmesh.ymlapplying when no token (env cleared in test).UnitTestson Linux.API / token note
A manual
GET https://api.qtmesh.dev/v1/ingest/ruleswith a test token returned 401 Unauthorized — that indicates an invalid, expired, or wrong-format ingest token on the qtmesh.dev side, not a client regression. Please confirm the ingest token from the dashboard matches what the CLI expects (e.g.Authorization: Bearer …). Do not commit tokens to the repo.Made with Cursor
Summary by CodeRabbit
New Features
Bug Fixes
Chores
Tests