Add diagnostics consent and fork-safe defaults#9
Merged
Conversation
|
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:
📝 Walkthrough<review_stack_artifact> 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
Comment |
duetCJackson
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/index.ts (1)
321-340:⚠️ Potential issue | 🟠 Major | ⚡ Quick winExpand Sentry event scrubbing beyond
homeDir.
beforeSend()only replaces the currenthomedir(), but the new controlled failure payload includes a raw email, an access token, and an absolute path that usually will not match the runner's real home directory. Those values can still be shipped in the Sentry event whenever analytics consent is enabled, even with diagnostic log upload disabled. Reuse the same redaction rules as the diagnostic-log attachment sanitizer here, or strip sensitive keys before capture.Also applies to: 1230-1241
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/index.ts` around lines 321 - 340, beforeSend currently only scrubs homeDir via scrubString and can leak emails, tokens, and absolute paths; update beforeSend/scrubString (used by deepScrub) to apply the same redaction rules as the diagnostic-log attachment sanitizer (or reuse that sanitizer) so Sentry events remove/replace raw emails, access tokens (e.g., Authorization headers, bearer tokens), and any absolute filesystem paths not matching homedir, and also strip sensitive keys like user.email, request.headers.authorization, and any attachment/payload fields before returning the event; keep the analyticsConsentEnabled guard and ensure delete event.server_name remains in place.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@e2e/diagnostic-log-upload.spec.ts`:
- Around line 7-14: The readStubEnvelopes function is brittle because it does a
single immediate read; change readStubEnvelopes to be retry-aware (polling with
a small delay and a total timeout) so it waits for the file to exist and contain
data before parsing; implement retries with exponential/backoff or fixed sleep
(e.g., check file presence/size then read, retry until success or timeout) and
surface a clear error on timeout; update any other direct calls that read the
same stub (the other occurrences noted in the comment) to use this retrying
helper so E2E runs are not flaky.
In `@src/main/notification-window.ts`:
- Around line 69-73: The title/body/primaryActionLabel assignments only escape
quotes and must also escape &, < and > to prevent HTML/script injection; create
or use a helper like escapeHtml that replaces & to &, < to <, > to >,
and also ' and " to &`#39`; / ", then apply it to the existing variables
(title, body, primaryActionLabel) and the other occurrences referenced (the
variables at the other block around lines 204-205) so all notification text
inserted into the data-URL is fully HTML-escaped.
- Around line 46-49: The current handlePrimaryAction calls
options.onPrimaryAction() then animateOut(), which skips the dismiss animation
if the primary callback throws; update handlePrimaryAction to invoke
options.onPrimaryAction() inside a try/finally (or try/catch/finally) so that
animateOut() is always executed in the finally block, and if you need to
preserve the original error propagate or rethrow it after animateOut; reference
handlePrimaryAction, options.onPrimaryAction, and animateOut when making the
change.
In `@src/main/services/__tests__/diagnostic-log-upload.test.ts`:
- Around line 29-49: The test uses a character-count check
(attachment?.data.length) which can miss UTF-8 multibyte size overflows; change
the assertion to check byte length instead by using
Buffer.byteLength(attachment.data, 'utf8') (or if attachment.data is already a
Buffer, use attachment.data.length) so the size assertion in the test for
buildDiagnosticLogAttachment verifies actual bytes (<= 64 * 1024) rather than
string length.
In `@src/main/services/__tests__/notes-ready-notifier.test.ts`:
- Around line 51-53: The test currently only checks that mocks.encryptJSON was
called and that mocks.showNotificationWindow was invoked; instead assert the
persisted dedupe payload by reading/inspecting the arguments passed to
mocks.encryptJSON (or the object written) to verify that
notesReadyNotificationSentAt exists and is a valid timestamp and that the target
path includes "metadata.json" (i.e., confirm the path string passed to
encryptJSON or the write call contains "metadata.json"); update the assertions
in notes-ready-notifier.test.ts to explicitly check the dedupe payload's
notesReadyNotificationSentAt field and the metadata.json target path alongside
the existing expectations.
In `@src/main/services/__tests__/segmentation.test.ts`:
- Around line 201-271: Add a regression test that ensures a throwing onComplete
callback does not mark segmentation as failed: in the segmentation.test.ts
create a case that mocks provider.summarize to resolve (successful
segmentation), registers an onComplete via service.onComplete that throws an
error when invoked, calls (service as any).processJob('m1') and asserts the
Promise resolves (or does not reject) and that the thrown error from the
callback does not propagate to cause processJob to fail; keep the same
transcript mocks and use vi.fn or a throwing function for onComplete to simulate
the callback error.
In `@src/main/services/diagnostic-log-upload.ts`:
- Around line 7-12: SECRET_PATTERNS currently matches key+delimiter+value and
replacements remove the whole match which can break JSON structure (e.g. turning
"access_token":"abc" into malformed text); update the token regexes and the
redact logic so only the token value is replaced and delimiters/keys remain
intact. Specifically, change the Bearer and token-field patterns in
SECRET_PATTERNS to capture the prefix/delimiter and the secret value separately
(e.g. /\bBearer\s+([A-Za-z0-9._~+/=-]+)\b/gi and
/\b(access|refresh|id)_token(["'=:\s]*)([A-Za-z0-9._~+/=-]+)/gi) and adjust the
replacement code in the redactSecrets function (the code using the patterns
around the previous lines 51-53) to rebuild the match using the captured
prefix/delimiter plus a fixed placeholder like "<REDACTED>" for the token group
so the JSON shape remains valid.
- Around line 30-36: The current truncation uses raw.length (UTF-16 code units)
instead of byte length; change the logic to enforce MAX_ATTACHMENT_BYTES in
UTF-8 bytes by converting raw to a Buffer (Buffer.from(raw, 'utf8')), then take
the last MAX_ATTACHMENT_BYTES bytes (buf.length > MAX_ATTACHMENT_BYTES ?
buf.slice(buf.length - MAX_ATTACHMENT_BYTES) : buf), convert that slice back to
a UTF-8 string (slice.toString('utf8')) and pass it into
sanitizeDiagnosticLogTail (keep variable names raw, tail, sanitized and constant
MAX_ATTACHMENT_BYTES to locate the code).
In `@src/main/services/notes-ready-notifier.ts`:
- Around line 30-45: The code currently writes the notesReadyNotificationSentAt
marker before attempting to display the notification (functions: encryptJSON,
notesReadyNotificationSentAt, MeetingMetadata, showNotificationWindow), which
can permanently suppress retries if showing the notification fails; change the
flow so you first call/show the notification (showNotificationWindow with
buildNotesReadyBody and getMeetingDisplayTitle) and only after that succeeds set
notesReadyNotificationSentAt on the MeetingMetadata and persist it via
encryptJSON; ensure you await or use the notification success callback (e.g.,
the onPrimaryAction/onDismiss completion) and do not write the marker on
notification failure so retries remain possible (use
focusMainWindow/getMainWindow as currently wired in the notification handlers).
In `@src/main/services/segmentation.ts`:
- Around line 250-251: Wrap the completion-callback invocation
this.onCompleteCallback?.(meetingId) in a try/catch so any exception thrown by
the callback is caught and logged instead of bubbling up and marking the job
failed (affects both occurrences around the current return and the one at the
other location). In the catch block log the error with the module's logger
(e.g., this.logger?.error or processLogger.error) and continue to return
normally so segmentation success flow in processNext is preserved; apply this
same change to the second call later in the file.
In `@src/main/services/sentry-stub.ts`:
- Around line 161-163: getCurrentScope currently always returns isolationScope
which prevents code inside withScope from seeing the active scoped data; change
getCurrentScope to return the actual active scope used by withScope (e.g., the
top of the scope stack or the currentScope variable) instead of the fixed
isolationScope so that tags/extras/attachments set in withScope are readable via
getCurrentScope(); update the function to retrieve the dynamic active scope
(falling back to isolationScope only if no active scope exists) and ensure it
references the same scope storage used by withScope.
In `@src/renderer/src/App.tsx`:
- Around line 299-301: The code handling the 'notes:open-meeting' event sets
window.location.hash using the raw meetingId which can contain characters that
break the URL; update the handler registered via
window.electronAPI.on('notes:open-meeting', ...) to URL-encode meetingId (use
encodeURIComponent on the meetingId value) before interpolating into
`#${ROUTES.recordings}/${meetingId}` so the hash route becomes
`#${ROUTES.recordings}/${encodeURIComponent(meetingId)}`, ensuring safe
navigation.
In `@src/renderer/src/pages/__tests__/Onboarding.test.tsx`:
- Around line 165-169: The test currently asserts that
'prefs:set-diagnostic-log-upload-consent' is true by default; change it to
assert explicit opt-in by expecting false (i.e.,
expect(window.electronAPI.invoke).toHaveBeenCalledWith('prefs:set-diagnostic-log-upload-consent',
false)), or alternatively simulate a user click on the log-upload checkbox in
the Onboarding test before asserting true; locate the call site using
window.electronAPI.invoke and the event name
'prefs:set-diagnostic-log-upload-consent' to update the expectation or add the
click interaction.
In `@src/renderer/src/pages/Onboarding.tsx`:
- Line 27: The state diagnosticLogUploadDraft in Onboarding is defaulting to
true which allows implied consent; change its initial value to false so the UI
starts unselected and only persists consent after the user explicitly enables
it, ensuring any logic that reads diagnosticLogUploadDraft (and updates it via
setDiagnosticLogUploadDraft) treats false as the safe default.
---
Outside diff comments:
In `@src/main/index.ts`:
- Around line 321-340: beforeSend currently only scrubs homeDir via scrubString
and can leak emails, tokens, and absolute paths; update beforeSend/scrubString
(used by deepScrub) to apply the same redaction rules as the diagnostic-log
attachment sanitizer (or reuse that sanitizer) so Sentry events remove/replace
raw emails, access tokens (e.g., Authorization headers, bearer tokens), and any
absolute filesystem paths not matching homedir, and also strip sensitive keys
like user.email, request.headers.authorization, and any attachment/payload
fields before returning the event; keep the analyticsConsentEnabled guard and
ensure delete event.server_name remains in place.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro (Legacy)
Run ID: dd970d77-dd81-46ed-86a4-32e667fd79d5
📒 Files selected for processing (38)
.github/workflows/build.ymle2e/diagnostic-log-upload.spec.tse2e/helpers/electron-app.tse2e/notes-ready-notification.spec.tse2e/onboarding-journey.spec.tssrc/main/index.tssrc/main/ipc/__tests__/prefs-ipc.test.tssrc/main/ipc/prefs-ipc.tssrc/main/notification-window.tssrc/main/services/__tests__/autodoc-log.test.tssrc/main/services/__tests__/calendar-oauth.test.tssrc/main/services/__tests__/detection.test.tssrc/main/services/__tests__/diagnostic-log-upload.test.tssrc/main/services/__tests__/distribution-config.test.tssrc/main/services/__tests__/notes-ready-notifier.test.tssrc/main/services/__tests__/segmentation.test.tssrc/main/services/__tests__/sentry-reporter.test.tssrc/main/services/autodoc-log.tssrc/main/services/calendar.tssrc/main/services/detection.tssrc/main/services/diagnostic-log-upload.tssrc/main/services/distribution-config.tssrc/main/services/microsoft-calendar.tssrc/main/services/notes-ready-notifier.tssrc/main/services/prefs-store.tssrc/main/services/segmentation.tssrc/main/services/sentry-reporter.tssrc/main/services/sentry-stub.tssrc/main/services/whisper-manager.tssrc/main/services/windows-transcription-runtime.tssrc/preload/ipc.d.tssrc/renderer/src/App.tsxsrc/renderer/src/components/onboarding/AnalyticsStep.tsxsrc/renderer/src/pages/Onboarding.tsxsrc/renderer/src/pages/Settings.test.tsxsrc/renderer/src/pages/Settings.tsxsrc/renderer/src/pages/__tests__/Onboarding.test.tsxsrc/shared/types.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/main/services/__tests__/segmentation.test.ts`:
- Around line 260-271: Remove the explicit any casts used when stubbing
fsMock.readFile in the segmentation tests; instead return a correctly typed
value (e.g., pass JSON.stringify([...]) directly if the mock is typed to return
string, or use Buffer.from(JSON.stringify([...])) if the mock expects a Buffer).
Update both occurrences where fsMock.readFile.mockResolvedValue is used (the
JSON.stringify([...]) calls around the test of segmentation) to return the
properly typed string or Buffer value and remove the `as any` casts.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro (Legacy)
Run ID: 301df285-fe09-4492-b107-c68f9ec3ac40
📒 Files selected for processing (13)
e2e/diagnostic-log-upload.spec.tssrc/main/notification-window.tssrc/main/services/__tests__/diagnostic-log-upload.test.tssrc/main/services/__tests__/notes-ready-notifier.test.tssrc/main/services/__tests__/segmentation.test.tssrc/main/services/__tests__/sentry-stub.test.tssrc/main/services/diagnostic-log-upload.tssrc/main/services/notes-ready-notifier.tssrc/main/services/segmentation.tssrc/main/services/sentry-stub.tssrc/renderer/src/App.tsxsrc/renderer/src/pages/Onboarding.tsxsrc/renderer/src/pages/__tests__/Onboarding.test.tsx
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/main/services/__tests__/whisper-onboarding-install.test.ts`:
- Line 9: The test suite is being entirely skipped in CI via the
describeWhisperOnboarding alias; change this so CI does not globally disable the
onboarding install tests: replace the conditional alias
(describeWhisperOnboarding) so it always maps to describe, and instead mark only
individually flaky tests with it.skip plus a TODO/tracking-issue comment (or add
a dedicated opt-out env flag like WHISPER_TESTS_SKIP to selectively skip).
Update uses of describeWhisperOnboarding to the real describe (or keep the alias
but remove the CI === 'true' branch) and add targeted skips for any known flaky
specs.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro (Legacy)
Run ID: d61ebc3d-2c36-4956-b963-2dbe9d4721e0
📒 Files selected for processing (1)
src/main/services/__tests__/whisper-onboarding-install.test.ts
This was referenced Jun 10, 2026
7 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.
What changed
Notes Ready, including durable per-meeting dedupe and navigation into the finished notesWhy
We wanted to make diagnostics consent more precise, keep privacy copy aligned with actual behavior, add a completion notification for finished notes, and stop forks from silently inheriting Duet-operated infrastructure by default.
Impact
Notes Readynotification when notes are fully preparedValidation
Notes Readynotification flowsnpm run buildnpm exec prettier -- --check .github/workflows/build.ymlNotes
AutoDoc-Landingprivacy page update lives in a separate repo and is not included in this PRscripts/macOSResetKeepMeetings.sh) was intentionally excluded from this branch