Add Sentry breadcrumbs to CLI pipeline - #188
Conversation
Initialize Sentry in CLI mode (using stored consent, no dialog) with launch_mode="cli" tag. Add breadcrumbs for each subcommand (info, convert, fix, anim) and captureMessage on errors. Wrap the CLI execution in a performance transaction. Flush Sentry before _exit(). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
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 (2)
📝 WalkthroughWalkthroughBumps project version to 2.11.3 and adds CLI telemetry via Sentry with a persistent opt-out flag Changes
Sequence Diagram(s)sequenceDiagram
participant User as CLI User
participant CLI as CLIPipeline
participant Settings as QSettings
participant Sentry as SentryReporter
participant Cmd as Command Handler
participant IO as Loader/Exporter
rect rgba(0,128,255,0.5)
User->>CLI: invoke CLI (with or without --no-telemetry)
CLI->>Settings: read telemetry preference / isFirstLaunch
CLI->>Sentry: init() [if not opted-out]
CLI->>Sentry: setTag(os, arch, qt_version, launch_mode)
CLI->>Sentry: startTransaction("cli")
end
CLI->>Sentry: addBreadcrumb(command)
CLI->>Cmd: dispatch(command)
Cmd->>IO: load/process/export
IO-->>Cmd: success/failure
alt failure
Cmd->>Sentry: captureException / addBreadcrumb(error)
end
Cmd-->>CLI: result
CLI->>Sentry: finishTransaction
CLI->>Sentry: shutdown()
CLI-->>User: exit code / output
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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: 5028657d2b
ℹ️ 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".
| redirectStdout(); | ||
|
|
||
| // Initialize Sentry using stored consent (no dialog in headless CLI mode) | ||
| SentryReporter::initialize(); |
There was a problem hiding this comment.
Initializing Sentry unconditionally in CLI mode causes telemetry to be enabled even when no consent has been recorded yet, because SentryReporter::isEnabled() defaults to true when Sentry/enabled is missing (src/SentryReporter.cpp), so a first-time headless run will immediately send cli.* transactions and breadcrumbs. This is a production privacy regression for users who run CLI before GUI consent flow (and on macOS it can also ignore GUI opt-out due to different QSettings identity), so CLI startup should skip Sentry until an explicit preference is present.
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: 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 `@src/CLIPipeline.cpp`:
- Around line 766-767: cmdAnim currently logs a breadcrumb but fails to emit
Sentry failure telemetry for individual import/merge and export error paths;
locate cmdAnim and add SentryReporter::captureMessage calls in the
per-merge-file import-failure branch (where merge files/mergeFile are skipped)
and in both export-failure branches (where export result or write fails),
emitting clear messages that include the operation ("anim import" or "anim
export"), the file name (fi.fileName() or mergeFile/file), and any available
error string/status; keep existing SentryReporter::addBreadcrumb calls, but
ensure each early return on merge import or export failure calls
SentryReporter::captureMessage with contextual text so anim failures are
observable.
- Around line 444-445: The breadcrumb currently sends raw filename and free-form
error text via SentryReporter::addBreadcrumb (e.g., the Inspect breadcrumb using
fi.fileName()); change these calls to avoid PII by sending only non-sensitive
metadata (command name, file extension/format, flags like jsonOutput, counts)
and a deterministic hash of the filename if you need a stable identifier; update
each SentryReporter::addBreadcrumb invocation (including the occurrences at the
listed lines) to build a sanitized message or structured data with keys such as
"command", "ext", "json", "count", and "file_hash" (compute a one-way hash of
fi.fileName() or error source instead of the plain name) and never include raw
error text — for errors, include an error code or sanitized summary only.
- Around line 405-410: The branch for unknown commands leaves rc == -1 and falls
through without calling SentryReporter::shutdown() or _exit(), so update the
end-of-function epilogue to always route through the same _exit() path: after
calling SentryReporter::finishTransaction(cliTxn), check if rc < 0 and if so set
a nonzero exit code (e.g., rc = EXIT_FAILURE or rc = 1) or call
SentryReporter::shutdown() and then _exit(rc); otherwise keep the existing
SentryReporter::shutdown()/_exit(rc) behavior—ensure SentryReporter::shutdown()
is invoked before any _exit() call so both unknown-command and normal paths use
the same teardown sequence.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7891ac60-89e0-4620-a6fa-784ce2362bc4
📒 Files selected for processing (2)
CMakeLists.txtsrc/CLIPipeline.cpp
- Gate Sentry init on explicit consent (skip if isFirstLaunch) - Replace filenames with extensions in breadcrumbs/errors to avoid PII - Route unknown commands through same _exit() epilogue with shutdown() - Add captureMessage for all anim error paths (merge import, merge export, rename export) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Replace silent consent gating with an explicit notice model: - First CLI run: print "usage data is collected" notice to stderr, enable telemetry, persist the preference - --no-telemetry: permanently opt out (stored in QSettings) - Subsequent runs: silent (telemetry on/off per stored preference) - Docker: works out of the box (defaults to enabled on first run) Add 10 new tests covering flag parsing, help text, consent logic, and opt-out persistence. Update README, CLAUDE.md, and docs/index.html. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/CLIPipeline.cpp (1)
349-372:⚠️ Potential issue | 🟠 MajorApply the telemetry preference handling before fast-return branches for --help, --version, and missing-command cases.
Lines 355-361 and lines 370-372 return before lines 392-399 execute. This means
qtmesh --no-telemetry --helpexits without persisting the opt-out preference to QSettings, and without printing the confirmation message. Future invocations will not recognize the opt-out.Additionally, the transaction and breadcrumb at lines 410-411 use unvalidated
cmdbefore the known-command check at lines 413-421. Move the preference handling to execute immediately after QApplication initialization (after line 376), before any early returns; validatecmdand create the transaction only after confirming it matches a known command.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/CLIPipeline.cpp` around lines 349 - 372, The telemetry opt-out handling (writing to QSettings and printing confirmation) must be moved to run immediately after QApplication is created (i.e., after the QApplication initialization block) so it always executes before any early returns; locate the current telemetry check that reads "--no-telemetry" (and uses QSettings and the confirmation message) and relocate its logic to just after QApplication initialization, ensuring it persists the preference and prints confirmation before any help/version/missing-command returns. Also defer creating the transaction/breadcrumb and using the `cmd` variable until after validating that `cmd` is a known command (move the transaction/breadcrumb creation that references `cmd` to after the known-command check), and ensure `cmdIndex`/`cmd` are validated against `argc` and the known-commands list before use.
♻️ Duplicate comments (1)
src/CLIPipeline.cpp (1)
409-410:⚠️ Potential issue | 🟠 MajorDon't send raw
cmdvalues to telemetry before validation.At Line 409 and Line 410,
cmdis still untrusted user input. A typo likeqtmesh /tmp/customer_model.fbxbecomes both the transaction name and breadcrumb, which reintroduces path/identifier leakage and high-cardinality telemetry. Normalize anything outside the supported subcommands to a constant such asunknownbefore logging it.🔒 Suggested fix
- auto cliTxn = SentryReporter::startTransaction("cli." + cmd, "cli.command"); - SentryReporter::addBreadcrumb("cli", QString("CLI command: %1").arg(cmd)); + const bool knownCmd = (cmd == "info" || cmd == "fix" || cmd == "convert" || cmd == "anim"); + const QString telemetryCmd = knownCmd ? cmd : "unknown"; + auto cliTxn = SentryReporter::startTransaction("cli." + telemetryCmd, "cli.command"); + SentryReporter::addBreadcrumb("cli", QString("CLI command: %1").arg(telemetryCmd));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/CLIPipeline.cpp` around lines 409 - 410, Sanitize and normalize the untrusted cmd before sending to telemetry: validate cmd against the allowed subcommands list and map anything not in that set to a constant like "unknown", then pass the normalized value to SentryReporter::startTransaction and SentryReporter::addBreadcrumb (i.e., perform validation/normalization of cmd prior to the calls to SentryReporter::startTransaction("cli."+cmd, ...) and SentryReporter::addBreadcrumb("cli", QString("CLI command: %1").arg(cmd))). Ensure the validation logic is implemented once (helper or inline) so only the safe normalized token is used for transaction names and breadcrumbs.
🧹 Nitpick comments (1)
src/CLIPipeline_test.cpp (1)
1599-1618: These test names overstate what is actually asserted.
HelpTextContainsNoTelemetryandNoTelemetryWithHelponly check the return code. They never verify help text or persisted opt-out behavior, so the names imply coverage that lives in the process-based tests below.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/CLIPipeline_test.cpp` around lines 1599 - 1618, The test names TEST(CLIPipelineRun, HelpTextContainsNoTelemetry) and TEST(CLIPipelineRun, NoTelemetryWithHelp) claim to verify help text and opt-out behavior but only assert the return code from CLIPipeline::run; either rename these tests to reflect they only assert exit code (e.g., HelpReturnsZero and NoTelemetryArgReturnsZero) or extend them to actually verify the behavior by capturing the help output and persisted opt-out state: for verifying help text, call the same help path that CLIPipeline::run uses (or invoke the process-based test helper) and assert stdout/stderr contains/does not contain "--no-telemetry"; for verifying opt-out persistence, after invoking CLIPipeline::run with "--no-telemetry" read the opt-out store/api used by CLIPipeline to assert the flag was saved. Ensure references to CLIPipeline::run and the two TEST names are updated accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/index.html`:
- Around line 736-740: Update the docs copy that currently implies
--no-telemetry is a one-time flag: change the sentence that references
<code>--no-telemetry</code> so it makes clear that choosing --no-telemetry
persists across runs (the preference is stored), for example by appending “(this
choice is saved and will apply to future runs)” or similar; update the centered
paragraph containing the mentions of <code>--verbose</code>,
<code>--no-telemetry</code>, and <code>--help</code> so the
<code>--no-telemetry</code> fragment explicitly notes persistence.
In `@src/CLIPipeline_test.cpp`:
- Around line 1668-1680: The fixture's QSettings is created without the CLI's
organization/application scope so it writes to the wrong location; in both
SetUp() and TearDown() call
QCoreApplication::setOrganizationName("QtMeshEditor") and
QCoreApplication::setApplicationName("QtMeshEditor") before constructing
QSettings so the tests operate on the same settings store used by CLIPipeline,
then continue to check/remove/set the "Sentry/enabled" key as currently
implemented in SetUp() and TearDown().
- Around line 1635-1662: The tests TEST(CLIPipelineCLI, NoTelemetryWithHelp) and
TEST(CLIPipelineCLI, NoTelemetryPrintsConfirmation) spawn the real binary and
can mutate the developer's QSettings store; before calling proc.start() set an
isolated environment so the child uses a temporary config dir (e.g., set
XDG_CONFIG_HOME or HOME to a temp path via QProcess::setEnvironment /
QProcess::setProcessEnvironment or equivalent) so QSettings writes go to a temp
location, or alternatively move these two tests into the existing
CLIPipelineTelemetryTest fixture which provides SetUp()/TearDown() that creates
and cleans a temporary settings directory; ensure the change is applied before
findAppBinary()/proc.start() so the child process never sees the real user
profile.
---
Outside diff comments:
In `@src/CLIPipeline.cpp`:
- Around line 349-372: The telemetry opt-out handling (writing to QSettings and
printing confirmation) must be moved to run immediately after QApplication is
created (i.e., after the QApplication initialization block) so it always
executes before any early returns; locate the current telemetry check that reads
"--no-telemetry" (and uses QSettings and the confirmation message) and relocate
its logic to just after QApplication initialization, ensuring it persists the
preference and prints confirmation before any help/version/missing-command
returns. Also defer creating the transaction/breadcrumb and using the `cmd`
variable until after validating that `cmd` is a known command (move the
transaction/breadcrumb creation that references `cmd` to after the known-command
check), and ensure `cmdIndex`/`cmd` are validated against `argc` and the
known-commands list before use.
---
Duplicate comments:
In `@src/CLIPipeline.cpp`:
- Around line 409-410: Sanitize and normalize the untrusted cmd before sending
to telemetry: validate cmd against the allowed subcommands list and map anything
not in that set to a constant like "unknown", then pass the normalized value to
SentryReporter::startTransaction and SentryReporter::addBreadcrumb (i.e.,
perform validation/normalization of cmd prior to the calls to
SentryReporter::startTransaction("cli."+cmd, ...) and
SentryReporter::addBreadcrumb("cli", QString("CLI command: %1").arg(cmd))).
Ensure the validation logic is implemented once (helper or inline) so only the
safe normalized token is used for transaction names and breadcrumbs.
---
Nitpick comments:
In `@src/CLIPipeline_test.cpp`:
- Around line 1599-1618: The test names TEST(CLIPipelineRun,
HelpTextContainsNoTelemetry) and TEST(CLIPipelineRun, NoTelemetryWithHelp) claim
to verify help text and opt-out behavior but only assert the return code from
CLIPipeline::run; either rename these tests to reflect they only assert exit
code (e.g., HelpReturnsZero and NoTelemetryArgReturnsZero) or extend them to
actually verify the behavior by capturing the help output and persisted opt-out
state: for verifying help text, call the same help path that CLIPipeline::run
uses (or invoke the process-based test helper) and assert stdout/stderr
contains/does not contain "--no-telemetry"; for verifying opt-out persistence,
after invoking CLIPipeline::run with "--no-telemetry" read the opt-out store/api
used by CLIPipeline to assert the flag was saved. Ensure references to
CLIPipeline::run and the two TEST names are updated accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d809a7c3-f70a-410a-9cfa-b3409e76ca9d
📒 Files selected for processing (5)
CLAUDE.mdREADME.mddocs/index.htmlsrc/CLIPipeline.cppsrc/CLIPipeline_test.cpp
✅ Files skipped from review due to trivial changes (1)
- README.md
| <p style="text-align: center; color: #aaa; font-family: 'Source Code Pro', monospace; margin-top: 30px;"> | ||
| Multiline examples use POSIX <code style="color: var(--primary-green);">\</code> continuations; on Windows, join into one line or adapt for PowerShell.<br> | ||
| Use <code style="color: var(--primary-green);">--verbose</code> for engine debug output • | ||
| <code style="color: var(--primary-green);">--no-telemetry</code> to opt out of anonymous usage data • | ||
| <code style="color: var(--primary-green);">--help</code> for full usage |
There was a problem hiding this comment.
Say that --no-telemetry persists across runs.
The implementation stores this preference, but this copy reads like a one-shot flag. Users can easily assume it only affects the current invocation.
✏️ Suggested wording
- <code style="color: var(--primary-green);">--no-telemetry</code> to opt out of anonymous usage data •
+ <code style="color: var(--primary-green);">--no-telemetry</code> to permanently opt out of anonymous usage data •📝 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.
| <p style="text-align: center; color: #aaa; font-family: 'Source Code Pro', monospace; margin-top: 30px;"> | |
| Multiline examples use POSIX <code style="color: var(--primary-green);">\</code> continuations; on Windows, join into one line or adapt for PowerShell.<br> | |
| Use <code style="color: var(--primary-green);">--verbose</code> for engine debug output • | |
| <code style="color: var(--primary-green);">--no-telemetry</code> to opt out of anonymous usage data • | |
| <code style="color: var(--primary-green);">--help</code> for full usage | |
| <p style="text-align: center; color: `#aaa`; font-family: 'Source Code Pro', monospace; margin-top: 30px;"> | |
| Multiline examples use POSIX <code style="color: var(--primary-green);">\</code> continuations; on Windows, join into one line or adapt for PowerShell.<br> | |
| Use <code style="color: var(--primary-green);">--verbose</code> for engine debug output • | |
| <code style="color: var(--primary-green);">--no-telemetry</code> to permanently opt out of anonymous usage data • | |
| <code style="color: var(--primary-green);">--help</code> for full usage |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/index.html` around lines 736 - 740, Update the docs copy that currently
implies --no-telemetry is a one-time flag: change the sentence that references
<code>--no-telemetry</code> so it makes clear that choosing --no-telemetry
persists across runs (the preference is stored), for example by appending “(this
choice is saved and will apply to future runs)” or similar; update the centered
paragraph containing the mentions of <code>--verbose</code>,
<code>--no-telemetry</code>, and <code>--help</code> so the
<code>--no-telemetry</code> fragment explicitly notes persistence.
| void SetUp() override { | ||
| QSettings settings; | ||
| m_hadSetting = settings.contains("Sentry/enabled"); | ||
| if (m_hadSetting) | ||
| m_previousValue = settings.value("Sentry/enabled").toBool(); | ||
| settings.remove("Sentry/enabled"); | ||
| } | ||
| void TearDown() override { | ||
| QSettings settings; | ||
| if (m_hadSetting) | ||
| settings.setValue("Sentry/enabled", m_previousValue); | ||
| else | ||
| settings.remove("Sentry/enabled"); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's check the CLIPipeline.cpp lines around 382-384
cat -n src/CLIPipeline.cpp | sed -n '375,390p'Repository: fernandotonon/QtMeshEditor
Length of output: 897
🏁 Script executed:
# Next, let's check the CLIPipeline_test.cpp lines around 1668-1680
cat -n src/CLIPipeline_test.cpp | sed -n '1665,1685p'Repository: fernandotonon/QtMeshEditor
Length of output: 843
🏁 Script executed:
# Let's also search for any QSettings initialization patterns in the test file
rg -n "QSettings" src/CLIPipeline_test.cpp | head -20Repository: fernandotonon/QtMeshEditor
Length of output: 158
🏁 Script executed:
# Check for organization/application setup in CLIPipeline or its initialization
rg -n "setApplicationName|setOrganizationName" src/CLIPipeline.cppRepository: fernandotonon/QtMeshEditor
Length of output: 194
QSettings fixture must match CLI organization/application scope.
The test fixture creates QSettings without setting organization/application names, causing it to persist to a different settings location than the CLI code, which explicitly sets organization to QtMeshEditor (lines 382-383 of CLIPipeline.cpp). Add QCoreApplication::setOrganizationName("QtMeshEditor") and QCoreApplication::setApplicationName("QtMeshEditor") before creating QSettings in both SetUp() and TearDown().
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/CLIPipeline_test.cpp` around lines 1668 - 1680, The fixture's QSettings
is created without the CLI's organization/application scope so it writes to the
wrong location; in both SetUp() and TearDown() call
QCoreApplication::setOrganizationName("QtMeshEditor") and
QCoreApplication::setApplicationName("QtMeshEditor") before constructing
QSettings so the tests operate on the same settings store used by CLIPipeline,
then continue to check/remove/set the "Sentry/enabled" key as currently
implemented in SetUp() and TearDown().
Docker containers are ephemeral so QSettings is wiped every run, causing the notice to print on every invocation. Add QTMESH_NO_TELEMETRY_NOTICE=1 env var in Dockerfile to suppress it. Native CLI users still see the one-time notice on first launch. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/CLIPipeline.cpp (1)
412-413:⚠️ Potential issue | 🟠 MajorDon’t send unvalidated
cmdvalues to Sentry.Line 412 starts telemetry with the raw
cmdbefore it is checked against the supported subcommands. On malformed invocations, that token can be a user filename/path or arbitrary text, which leaks user data back into Sentry and creates high-cardinality transaction names. Normalize unknown commands to a fixed value likeunknownbefore callingstartTransaction()/addBreadcrumb().Proposed fix
- auto cliTxn = SentryReporter::startTransaction("cli." + cmd, "cli.command"); - SentryReporter::addBreadcrumb("cli", QString("CLI command: %1").arg(cmd)); + const bool knownCommand = + (cmd == "info" || cmd == "fix" || cmd == "convert" || cmd == "anim"); + const QString telemetryCmd = knownCommand ? cmd : "unknown"; + + auto cliTxn = SentryReporter::startTransaction("cli." + telemetryCmd, "cli.command"); + SentryReporter::addBreadcrumb("cli", QString("CLI command: %1").arg(telemetryCmd));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/CLIPipeline.cpp` around lines 412 - 413, The telemetry call is using the raw cmd token (variable cmd) which may leak user data; before calling SentryReporter::startTransaction(...) and SentryReporter::addBreadcrumb(...), validate cmd against the list of supported subcommands and replace any value not in that allowlist with a fixed sentinel like "unknown" (or normalize to a safe label) so startTransaction("cli."+cmd, ...) and addBreadcrumb("cli", ...) only ever receive validated/normalized command names.
🤖 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 340-345: The static CLI-scoped flags s_verbose and s_noTelemetry
must be reset at the start of CLIPipeline::run() so prior invocations don't leak
state; modify run() to explicitly set s_verbose = false and s_noTelemetry =
false (or their default values) before the pre-scan loop that inspects argv,
ensuring the pre-scan then correctly sets them for the current invocation.
---
Duplicate comments:
In `@src/CLIPipeline.cpp`:
- Around line 412-413: The telemetry call is using the raw cmd token (variable
cmd) which may leak user data; before calling
SentryReporter::startTransaction(...) and SentryReporter::addBreadcrumb(...),
validate cmd against the list of supported subcommands and replace any value not
in that allowlist with a fixed sentinel like "unknown" (or normalize to a safe
label) so startTransaction("cli."+cmd, ...) and addBreadcrumb("cli", ...) only
ever receive validated/normalized command names.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2bd9a162-5757-4c47-9806-094f56fd31ab
📒 Files selected for processing (2)
Dockerfilesrc/CLIPipeline.cpp
| // Pre-scan for --verbose and --no-telemetry before anything else | ||
| for (int i = 1; i < argc; ++i) { | ||
| if (QString(argv[i]) == "--verbose") { | ||
| s_verbose = true; | ||
| break; | ||
| } | ||
| QString arg(argv[i]); | ||
| if (arg == "--verbose") s_verbose = true; | ||
| if (arg == "--no-telemetry") s_noTelemetry = true; | ||
| } |
There was a problem hiding this comment.
Reset CLI-scoped flags at the start of run().
Line 344 can leave s_noTelemetry stuck true for every later CLIPipeline::run() call in the same process. That makes tests and any embedded reuse inherit a previous invocation’s opt-out state.
Proposed fix
int CLIPipeline::run(int argc, char* argv[])
{
+ s_verbose = false;
+ s_noTelemetry = false;
+
// Pre-scan for --verbose and --no-telemetry before anything else
for (int i = 1; i < argc; ++i) {
QString arg(argv[i]);
if (arg == "--verbose") s_verbose = true;
if (arg == "--no-telemetry") s_noTelemetry = true;📝 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.
| // Pre-scan for --verbose and --no-telemetry before anything else | |
| for (int i = 1; i < argc; ++i) { | |
| if (QString(argv[i]) == "--verbose") { | |
| s_verbose = true; | |
| break; | |
| } | |
| QString arg(argv[i]); | |
| if (arg == "--verbose") s_verbose = true; | |
| if (arg == "--no-telemetry") s_noTelemetry = true; | |
| } | |
| s_verbose = false; | |
| s_noTelemetry = false; | |
| // Pre-scan for --verbose and --no-telemetry before anything else | |
| for (int i = 1; i < argc; ++i) { | |
| QString arg(argv[i]); | |
| if (arg == "--verbose") s_verbose = true; | |
| if (arg == "--no-telemetry") s_noTelemetry = true; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/CLIPipeline.cpp` around lines 340 - 345, The static CLI-scoped flags
s_verbose and s_noTelemetry must be reset at the start of CLIPipeline::run() so
prior invocations don't leak state; modify run() to explicitly set s_verbose =
false and s_noTelemetry = false (or their default values) before the pre-scan
loop that inspects argv, ensuring the pre-scan then correctly sets them for the
current invocation.
- docs/index.html: say "permanently" for --no-telemetry description - CLIPipelineTelemetryTest: set org/app name to match CLI's QSettings scope - QProcess telemetry tests: use isolated temp HOME to avoid mutating developer's real QSettings - Fix NoTelemetryPrintsConfirmation: use real subcommand instead of --help (which exits before telemetry code runs) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
1 participant
Summary
launch_mode="cli"tag (uses stored consent, no dialog in headless mode)cli.info,cli.convert,cli.fix,cli.animcli.<command>)_exit()to ensure events are sentTest plan
🤖 Generated with Claude Code
Summary by CodeRabbit
Chores
New Features
Improvements
Documentation
Tests