Fix multi-agent review findings from PR #73#74
Merged
Conversation
Aggregated Critical/High findings from /total-review (7 agents) on the Premium TTS subscription PR. Bundled into one atomic fix commit. Worker (workers/hdzap-premium): - appleJws.ts: anchor the x5c chain to the embedded Apple Root CA G3 by DER equality (not just fingerprint match), and check every cert's validity window so expired intermediates can't pass. Closes the bypass where an attacker-controlled chain with the public Apple Root tacked on top would satisfy the previous fingerprint-only check. - index.ts: drop the wrong "X-HDZap-SampleRate: 22050" header on the Polly miss path (Polly Neural PCM is always 16 kHz — the 22050 leftover would pitch-shift cache hits 37% if iOS ever started honouring the header). - index.ts: reject upstream 200 responses whose Content-Type isn't audio/* (Polly/Azure) or text/event-stream (Cartesia). Without this an Azure maintenance page or Cartesia plaintext quota notice would be served to iOS as PCM and play as static. - Drop stale "backwards compat with iOS build that doesn't yet send provider" comment + clarify the deferred `userId` binding. iOS Speech (app/HDZap/Models/Speech, app/HDZap/Models): - PremiumSpeechSynthesizer: speakAsync() now tears down audio via cancel() before spawning a new task. Previously only Task.cancel() ran, which left the player node + AVAudioEngine running and the prior utterance's tail bled into the new voice's head. - PremiumSpeechSynthesizer: cancel() now stops the engine and bumps a per-utterance generation counter; scheduled-buffer completion callbacks ignore decrements whose captured generation no longer matches, so a stale callback from a superseded utterance can't flip isPlaying false on a fresh one. cancel() also resets accumulatedPCM so a previous partial stream can't bleed into the next entry. - PremiumSpeechSynthesizer: new onUtteranceEnd callback fires exactly once per utterance end (drain / cancel / error). LapAnnouncer wires it to utteranceDidEnd() so Premium playback now participates in the same inflight-utterance bookkeeping the System path always had — the warm- keeper engine and session-hold lifecycle were silently broken on the Premium branch (inflightUtteranceCount stayed at 0 forever). - PremiumSpeechSynthesizer: trim odd trailing byte before writing PCM to TTSCache (the cached file would otherwise have a 0.5-sample garbage tail that played back as a click on every cache hit). - PremiumSpeechSynthesizer: throw streamFailure when Cartesia SSE drains with zero audio chunks despite non-zero lines — provider schema drift would otherwise silently fall back to System voice with no signal. - LapAnnouncer: increment inflightUtteranceCount on the Premium branch and configure the shared audio session up front; Premium now matches the System path's lifecycle. iOS IAP (app/HDZap/Models/IAP): - SubscriptionManager: pick the strongest entitlement (active > grace, then latest expiry) instead of whichever transaction the loop visited last. Apple can hand back multiple entitlements on monthly→yearly upgrades. - SubscriptionManager: clear currentJWS once a grace-period transaction is more than 16 days past its expiry so we stop pinging the Worker with a token it'll reject. SubscriptionStatus.inGracePeriod now carries a non-optional Date (grace by definition implies a known expiry). - SubscriptionManager: purchase() populates lastError before re-throwing so the paywall can surface the error without callers plumbing it. - SubscriptionManager: .unverified branch sets lastError so a tampered transaction shows a banner instead of leaving the operator wondering why Subscribe did nothing. iOS Views (app/HDZap/Views/Settings): - PaywallView: drop the misleading comment that claimed SubscriptionManager.lastError was set inside purchase() (it now is). - AudioSettingsView: drop stale "before that wiring lands" reference. Privacy + CI (docs, .github): - Fix the broken Cartesia privacy link in both EN and JA pages (https://cartesia.ai/privacy → /legal/privacy.html — the old URL returns 404, which is an App Privacy review smell). - Guard each manual-page cp in flasher.yml so a page added on one branch but not the other doesn't break the composite build. Misc: - Sync app/HDZap.xcodeproj/project.pbxproj to the 1.1.0/12 versions in project.yml (xcodegen drift after PR #73 merged). - Drop iOS 26 typo + 2026-05-19 datestamp + Phase 2 SpeechRouter references that all rotted after PR #73 landed. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ 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: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (7)
Walkthroughクライアント側の複数権利ランク付けロジック、Worker側の証明書検証強化、PremiumSpeechSynthesizerの世代管理再設計、およびLapAnnouncerとの統合により、JWS不整合排除、無効トークン送信回避、再生ライフサイクル不整合防止が実装されます。ビルド版更新とドキュメント統一により一貫性を確保。 ChangesJWS認証と権利選択ロジック
PremiumSpeechSynthesizer ライフサイクル管理
LapAnnouncerおよびドキュメント・設定更新
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 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. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/HDZap/Models/IAP/SubscriptionManager.swift (1)
107-137:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
lastErrorを購入開始時にクリアしてください。このメソッドで
lastErrorを保持するようになった一方、次の購入開始時や.success/.userCancelled/.pendingでは消していないので、直前の失敗メッセージが paywall に残り続けます。エラー伝播を UI に出す契約なら、非エラー終了後に古いエラーが見え続ける状態は避けたいです。💡 最小修正案
func purchase(_ product: Product) async throws -> Bool { + lastError = nil purchasing = product.id defer { purchasing = nil } let result: Product.PurchaseResult🤖 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 `@app/HDZap/Models/IAP/SubscriptionManager.swift` around lines 107 - 137, Clear stale error state by resetting lastError at the start of purchase(_ product: Product) and ensure it is cleared on non-error completion paths: set lastError = nil before returning in the .success (after await handle(verificationResult:) / await refreshEntitlement()), .userCancelled, and .pending branches so the previous failure message does not remain visible; keep existing logging and error handling for the catch block that populates lastError on actual failures.
🤖 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 `@app/HDZap/Models/LapAnnouncer.swift`:
- Around line 332-338: The premiumSynth.onUtteranceEnd closure currently only
calls utteranceDidEnd(), causing AVAudioSession to remain active; update the
closure (premiumSynth.onUtteranceEnd) to mirror the System synth path by calling
utteranceDidEnd() and then invoking deactivateSession() when appropriate (e.g.,
only if self?.sessionHoldActive == false) so the shared AVAudioSession is
released after Premium playback finishes.
In `@app/HDZap/Models/Speech/PremiumSpeechSynthesizer.swift`:
- Around line 282-290: The notifyEnd() early-return on guard isPlaying causes
missing end notifications when an utterance was accepted but never started
(cancel/error/overwrite before playback), leaking LapAnnouncer's inflight count;
add a separate boolean (e.g. utteranceAccepted or hasPendingUtterance) tracked
when the utterance is accepted (where speakAsync/cancel paths mark it) and
change notifyEnd() to check and clear that flag (and still clear isPlaying if
set) so onUtteranceEnd() is invoked exactly once per accepted utterance; update
speakAsync, cancel, and any error/overwrite handlers to set/clear this new flag
consistently.
In `@app/HDZap/Models/Speech/TTSCache.swift`:
- Around line 13-17: Update the conflicting comment section so the capacity
estimate matches the PCM caching policy described in the header ("Every cached
payload is raw s16le mono PCM..."). Replace the mp3-based size assumptions in
the later comment string that mentions "Polly/Azure mp3" with PCM math using
each provider's native sample rate (Polly 16 kHz, Cartesia/Azure 24 kHz), mono
channel count, and 2 bytes per sample (s16le): bytes ≈ sampleRate *
duration_seconds * 1 * 2. Remove any references to mp3 bitrate and ensure the
explanatory text (the comment that currently assumes mp3 sizes) clearly states
the PCM calculation and example numbers for common utterance lengths.
In `@workers/hdzap-premium/src/index.ts`:
- Around line 182-185: Remove the silent fallback to "cartesia" so missing
provider requests don't proceed upstream: stop using (body.provider ||
"cartesia") in the assignment to provider in index.ts and instead validate
body.provider exists first; if absent, return the proper 400 error (with an
explicit message naming the missing field) rather than normalizing and
continuing, and when present use provider = body.provider.trim().toLowerCase();
ensure this change affects the same logic that interacts with
PremiumSpeechSynthesizer/upstream so no request without an explicit provider is
forwarded.
- Around line 280-292: The current Content-Type check inspects the wrapped
Response (`upstream`) set by proxyCartesia/proxyPolly/proxyAzure and so misses
bad upstream bodies; instead, inside each proxy function (proxyCartesia,
proxyPolly, proxyAzure) validate the raw provider response via
resp.headers.get("Content-Type") before you rewrap it: compute expectedPrefix
(audio/ or text/event-stream for cartesia) and if resp.headers.get(...) doesn't
startWith that prefix, log the provider and raw content-type and return the
502/json error immediately (same shape currently returned), so you never wrap
and cache a non-audio body as audio. Ensure the existing upstream check is
removed or kept as a redundant safeguard only after proxies perform the real
validation.
---
Outside diff comments:
In `@app/HDZap/Models/IAP/SubscriptionManager.swift`:
- Around line 107-137: Clear stale error state by resetting lastError at the
start of purchase(_ product: Product) and ensure it is cleared on non-error
completion paths: set lastError = nil before returning in the .success (after
await handle(verificationResult:) / await refreshEntitlement()), .userCancelled,
and .pending branches so the previous failure message does not remain visible;
keep existing logging and error handling for the catch block that populates
lastError on actual 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 615f452b-ce98-4a61-b6a0-f2e47752b0db
📒 Files selected for processing (13)
.github/workflows/flasher.ymlapp/HDZap.xcodeproj/project.pbxprojapp/HDZap/Models/IAP/SubscriptionManager.swiftapp/HDZap/Models/LapAnnouncer.swiftapp/HDZap/Models/Speech/BuildSecrets.swift.tplapp/HDZap/Models/Speech/PremiumSpeechSynthesizer.swiftapp/HDZap/Models/Speech/TTSCache.swiftapp/HDZap/Views/Settings/AudioSettingsView.swiftapp/HDZap/Views/Settings/PaywallView.swiftdocs/manual/_pages/privacy/index.htmldocs/manual/_pages/privacy/ja/index.htmlworkers/hdzap-premium/src/appleJws.tsworkers/hdzap-premium/src/index.ts
Critical regressions from the previous fix commit: - Premium speakAsync re-entry races: the catch handler called `self.cancel()` unconditionally, which would tear down the NEW currentTask if a fresh speakAsync replaced the cancelled one before its catch ran. Catch now captures the generation token at speakAsync entry and bails when the generation has been bumped (i.e., we've been superseded). - Inflight counter leak: `notifyEnd`'s `guard isPlaying` swallowed the end notification when `speak()` threw BEFORE setting `isPlaying = true` (`bearer.isEmpty`, bad URL, `configureSession()` failure). The companion fix moves `isPlaying = true` to `speakAsync` BEFORE spawning the Task so any early error still produces a clean end notification, and notifyEnd no longer guards on isPlaying — it fires pendingOnEnd at most once per speakAsync invocation regardless of whether playback actually started. - Picker / paywall preview counter underflow: PaywallView and PremiumVoicePickerView call `announcer.premiumSynth.speakAsync(...)` directly, bypassing `LapAnnouncer.speak`'s inflight increment. The global `onUtteranceEnd` callback that fix-PR-#73 wired into LapAnnouncer would have fired `utteranceDidEnd()` on every preview end, driving the counter negative (debug crash via underflow assertion; release lifecycle corruption). Replaced the global callback with a per-call `onEnd:` parameter on speakAsync — LapAnnouncer.speak passes a decrement closure, preview surfaces don't, so previews neither increment nor decrement. - Premium end now also calls `deactivateSession()` symmetric with the System path's `didFinish` / `didCancel` delegate — PR-#73's wiring only decremented the counter, leaving the audio session active until the next System utterance happened to deactivate it. Worker: - `provider` defaulting to `"cartesia"` on missing field forwarded malformed requests to a real (billable) upstream call. Reject with `400 missing-provider` instead. - Content-type guard moved INSIDE each proxy function so it checks the actual provider response, not Worker-wrapped headers. The previous placement read our own normalized Content-Type and was a no-op against the original concern (Azure maintenance HTML, Cartesia plaintext quota notice). Polly accepts `audio/*` or `application/*; ...pcm...` because AWS has tweaked the type string in the past. - Cert-chain DER comparison uses `Buffer.from(...).equals(...)` so it works whether `X509Certificate.raw` is a Buffer or an ArrayBuffer (Workers `nodejs_compat` has varied between releases). Indexing a raw ArrayBuffer with `[i]` returns undefined and would have made every Premium JWS reject. Misc: - TTSCache capacity-estimate comment matched the new raw-PCM layout (32-48 KB/sec depending on sample rate) instead of the old mp3 sizing. - Comment polish per the comment-analyzer (drop dead `_ = error`, trim "previous behavior" historical reference in refreshEntitlement, tighten verifyChain doc parenthetical, remove `void userId` dead binding — userId is genuinely used in the auth log line above). - `cancel()` doc calls out the `&+=` overflow-wrap choice explicitly so a future reader doesn't read it as a hand-rolled bug. Verification: `xcodebuild build -configuration Debug` succeeds locally. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Saqoosha
added a commit
that referenced
this pull request
May 20, 2026
Saqoosha
pushed a commit
that referenced
this pull request
May 21, 2026
Resolves conflicts against develop's StoreKit2 paywall (PR #73 + #74) on top of the maintainer's review-feedback follow-ups for PR #60. Subscription gate: - TimerView.publishWatchSnapshot sends hapticsEnabled = (watchHapticsEnabled && subscription.isEntitled), so the watch only acts on what the iPhone says. Single gate — no parallel check on the wrist. - PublishWatchSnapshotModifier picks up subscription.isEntitled as a tracked input, and the .onChange handler clears watchHapticsEnabled when entitlement lapses (mirrors the TTS-engine rollback in AudioSettingsView). - WatchSettingsView replaces the toggle (and the Try-haptics audition section) with a "Subscribe to enable" paywall card when not entitled. PaywallView presented via .sheet. - SettingsView's row summary shows "Premium" for non-subscribers. Conflict resolution notes: - project.pbxproj: took ours (xcodegen will regenerate from project.yml after merge). - Localizable.xcstrings: took theirs to keep develop's full Premium TTS translation catalog. Watch-specific keys regenerate on next build.
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
Aggregated Critical/High findings from a
/total-reviewpass (7 agents: pr-review-toolkit code-reviewer × 2, silent-failure-hunter, comment-analyzer, type-design-analyzer, feature-dev code-reviewer, coderabbit, codex) on PR #73. Bundled into one atomic fix commit per project convention.Worker — Critical
appleJws.tschain anchoring: x5c chain now DER-equality-checked against the embedded Apple Root CA G3, not just fingerprint-matched. Closes the bypass where an attacker-controlled chain with the public Apple Root tacked on top could pass.appleJws.tsvalidity windows: every cert in the chain is checked fornotBefore/notAfterso an expired Apple intermediate can't slip through.index.tsPolly sample-rate header:X-HDZap-SampleRate: 22050→16000(Polly Neural PCM is only ever 16 kHz; the 22050 leftover would have pitch-shifted cache hits 37% if iOS started honouring it).index.tscontent-type guard: upstream 200 responses whose Content-Type isn'taudio/*(Polly/Azure) ortext/event-stream(Cartesia) are downgraded to 502 — protects iOS from playing an Azure maintenance page as PCM.iOS — Critical
PremiumSpeechSynthesizer.speakAsyncre-entry: now calls fullcancel()first instead of justcurrentTask.cancel(). Previously the prior utterance's player node, engine, and scheduled buffers all kept running and bled into the head of the new voice mid-preview.PremiumSpeechSynthesizer.cancel: stops the AVAudioEngine + bumps a per-utterance generation counter. Scheduled-buffer completion callbacks now ignore decrements whose captured generation is stale, so a queued callback from a cancelled utterance can't flipisPlaying = falseon a fresh one.LapAnnouncerPremium branch: incrementsinflightUtteranceCountbefore callingspeakAsyncand decrements via a newPremiumSpeechSynthesizer.onUtteranceEndcallback. The warm-keeper engine and session-hold lifecycle were silently broken on the Premium branch (counter stuck at 0 forever).iOS — High
SubscriptionManager.refreshEntitlement: picks the strongest entitlement (active > grace, then latest expiry) instead of whichever transactionTransaction.currentEntitlementshappened to yield last. Apple can return both monthly + yearly on upgrades.SubscriptionManager: clearscurrentJWSwhen a grace-period transaction is >16 days past its expiry.SubscriptionStatus.inGracePeriodcarries a non-optionalDate(grace implies a known expiry by definition).SubscriptionManager.purchase/.unverified: both now populatelastErrorso the paywall can surface failures.PremiumSpeechSynthesizer.playPCMFromStream+parseSSE: trim odd trailing byte before writing PCM toTTSCache(would have produced a 0.5-sample click on every cache hit). Cartesia SSE drain with zero chunks but non-zero lines now throwsstreamFailureso provider schema drift surfaces instead of silently falling back to System voice.Docs + CI
/privacyreturned 404, correct is/legal/privacy.html— App Privacy review smell).cpinflasher.ymlso a page added on one branch but not the other doesn't break the composite build (we hit this with the JA privacy page after the merge of Premium TTS subscription (AWS Polly, Azure, Cartesia) #73).Misc
app/HDZap.xcodeproj/project.pbxprojto the 1.1.0/12 versions already inproject.yml(xcodegen drift after Premium TTS subscription (AWS Polly, Azure, Cartesia) #73).Phase 2 SpeechRouterreferences that rotted with Premium TTS subscription (AWS Polly, Azure, Cartesia) #73 merging.Verification
xcodebuild build -configuration Debug -destination 'generic/platform=iOS'✅ BUILD SUCCEEDED locally on this branch.Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
Bug Fixes
Documentation
Chores