Extract dogfood feedback sink from TerminalController into CmuxDogfoodFeedbackSink - #6168
Closed
azooz2003-bit wants to merge 1 commit into
Closed
azooz2003-bit wants to merge 1 commit into
azooz2003-bit wants to merge 1 commit into
Conversation
…dFeedbackSink Moves the privileged Mac<->phone dogfood feedback persistence domain out of the TerminalController god file into a new leaf package, CmuxDogfoodFeedbackSink (zero deps, mirrors CmuxMobileSupport's manifest shape). What moved: - The field/blob size caps and bundle-retention cap (now DogfoodFeedbackLimits, a Sendable value type; .default carries the exact prior constants). - The privileged-domain email gate (isPrivilegedFeedbackEmail, now a static on DogfoodFeedbackService). - The validate + off-main decode + bundle write + prune logic (now DogfoodFeedbackService.submit, a nonisolated Sendable Service). - The write outcome type (now DogfoodFeedbackOutcome, one case per RPC response). Seams: FileManager (via an @sendable provider, since FileManager isn't Sendable and the writer runs on a detached task), the cache root URL, and the clock are constructor-injected with production defaults, so tests drive it against a temp dir with a fixed timestamp. App side: v2MobileDogfoodFeedbackSubmit is now a thin forward. It still resolves the authenticated email via the main-actor MobileHostService.shared, reads the four wire fields via v2RawString, calls service.submit(...), and maps the outcome to V2CallResult. The service re-enforces the @manaflow.ai gate at the boundary. Byte-identical: same caps, same base64-char-cap-before-decode and byte-cap ordering, same Task.detached(.utility) off-main structure, same bundle dir naming (ISO8601 with colons -> '-', 8-char lowercase uuid), same 0700/0600 perms, same bundle.json schema/keys/sorted-keys/pretty, same lexicographic prune keeping newest 50, and the same RPC error codes/messages and .ok payload keys. Tests: 7 behavior tests (privilege gate, unauthorized-before-IO, base64 char cap, decoded byte cap, bundle write + manifest + perms, field capping, prune) with injected temp dir + fixed clock; swift test green. Removed ~147 lines from TerminalController.swift (14785 -> 14638); budget ratcheted. Wired the package into cmux.xcodeproj (5 pbxproj entries mirroring CmuxFeedback) and the app target import. NOTE: full app build validated by CI. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughA new local Swift package ChangesCmuxDogfoodFeedbackSink package extraction
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 20 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (20 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
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 |
Contributor
Greptile SummaryExtracts the dogfood feedback persistence domain out of
Confidence Score: 4/5The refactoring is structurally sound — the privilege gate, caps, and filesystem layout are all faithfully reproduced. Both findings are non-functional: a double now() call that can produce a cosmetic timestamp skew between the bundle directory name and the manifest, and an inherited docstring whose actor-isolation claim became incorrect when the code moved out of TerminalController. The double now() call means the manifest received_at and the bundle directory name can have different ISO8601 second values at a clock-second boundary. This has always been latent in the original code, and the injected clock makes it trivially fixable. The docstring now incorrectly says caps run on the calling actor — they run on the cooperative pool in Swift 6, which is actually an improvement, but the comment can mislead future readers about where that code executes. Packages/CmuxDogfoodFeedbackSink/Sources/CmuxDogfoodFeedbackSink/DogfoodFeedbackService.swift — double now() call and actor-isolation docstring. Important Files Changed
Sequence DiagramsequenceDiagram
participant Phone as Phone (RPC caller)
participant TC as TerminalController<br/>(@MainActor)
participant MHS as MobileHostService
participant SVC as DogfoodFeedbackService<br/>(cooperative pool)
participant DT as Task.detached<br/>(.utility)
participant FS as FileSystem
Phone->>TC: v2MobileDogfoodFeedbackSubmit(params)
TC->>MHS: currentAuthenticatedLocalUserEmail()
MHS-->>TC: localEmail
TC->>SVC: await submit(submission, authenticatedEmail: localEmail)
SVC->>SVC: isPrivilegedFeedbackEmail() gate
SVC->>SVC: cap text/terminal/buildStamp fields
SVC->>SVC: "guard base64.count <= maxBlobBase64Chars"
SVC->>DT: Task.detached(priority: .utility)
DT->>DT: Data(base64Encoded:) decode
DT->>DT: "guard decoded.count <= maxBlobBytes"
DT->>FS: createDirectory (0700) + write files (0600)
DT->>FS: pruneBundles(keep: 50)
DT-->>SVC: DogfoodFeedbackOutcome
SVC-->>TC: .written / .unauthorized / .invalidParams / .internalError
TC-->>Phone: V2CallResult (.ok or .err)
Reviews (1): Last reviewed commit: "Extract dogfood feedback sink from Termi..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+140
to
+151
| ) -> DogfoodFeedbackOutcome { | ||
| let root = cacheRoot | ||
|
|
||
| let formatter = ISO8601DateFormatter() | ||
| formatter.formatOptions = [.withInternetDateTime] | ||
| // Colons are legal in HFS+/APFS but awkward in shell globs; swap for `-` | ||
| // so the directory name is paste-safe. | ||
| let timestamp = formatter.string(from: now()).replacingOccurrences(of: ":", with: "-") | ||
| let shortID = String(UUID().uuidString.prefix(8)).lowercased() | ||
| let bundleDir = root.appendingPathComponent("\(timestamp)_\(shortID)", isDirectory: true) | ||
|
|
||
| do { |
Contributor
There was a problem hiding this comment.
now() is called twice in writeBundle — once for the directory name and again for received_at in the manifest. In production (where now is { Date.now }), if a second boundary is crossed between the two calls, the manifest's received_at field will not match the timestamp embedded in the bundle directory name. Now that the clock is injected, a single let ts = now() at the top of the function eliminates the skew for free.
Suggested change
| ) -> DogfoodFeedbackOutcome { | |
| let root = cacheRoot | |
| let formatter = ISO8601DateFormatter() | |
| formatter.formatOptions = [.withInternetDateTime] | |
| // Colons are legal in HFS+/APFS but awkward in shell globs; swap for `-` | |
| // so the directory name is paste-safe. | |
| let timestamp = formatter.string(from: now()).replacingOccurrences(of: ":", with: "-") | |
| let shortID = String(UUID().uuidString.prefix(8)).lowercased() | |
| let bundleDir = root.appendingPathComponent("\(timestamp)_\(shortID)", isDirectory: true) | |
| do { | |
| ) -> DogfoodFeedbackOutcome { | |
| let root = cacheRoot | |
| let formatter = ISO8601DateFormatter() | |
| formatter.formatOptions = [.withInternetDateTime] | |
| let ts = now() | |
| // Colons are legal in HFS+/APFS but awkward in shell globs; swap for `-` | |
| // so the directory name is paste-safe. | |
| let timestamp = formatter.string(from: ts).replacingOccurrences(of: ":", with: "-") | |
| let shortID = String(UUID().uuidString.prefix(8)).lowercased() | |
| let bundleDir = root.appendingPathComponent("\(timestamp)_\(shortID)", isDirectory: true) | |
| do { |
| try fileManager.setAttributes([.posixPermissions: 0o600], ofItemAtPath: diagnosticURL.path) | ||
| let manifest: [String: Any] = [ | ||
| "schema": "cmux.dogfood.feedback.v1", | ||
| "received_at": formatter.string(from: now()), |
Contributor
There was a problem hiding this comment.
now() call in the same function — use the same ts captured at the top (per the companion suggestion above) so the manifest's received_at is guaranteed to match the bundle directory name.
Suggested change
| "received_at": formatter.string(from: now()), | |
| "received_at": formatter.string(from: ts), |
Comment on lines
+66
to
+71
| /// onto an RPC response. | ||
| /// | ||
| /// The privilege check and the cheap per-field character caps run on the | ||
| /// calling actor; an oversized base64 blob is rejected here without | ||
| /// decoding. The decode plus filesystem writes run on a detached utility | ||
| /// task so a large payload never blocks the caller. |
Contributor
There was a problem hiding this comment.
The doc says "the cheap per-field character caps run on the calling actor," but in Swift 6, calling a nonisolated async method on a Sendable struct from a @MainActor context suspends the main actor and resumes on the global cooperative thread pool — the caps never execute on the main actor. This is actually better for UI responsiveness, but the comment directly contradicts that behavior and could mislead future readers into assuming main-actor execution for the field-cap code. The original caps did run on the main actor (they were inlined in TerminalController), so this docstring was inherited from a context where the claim was true.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
![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 current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmux.xcodeproj/project.pbxproj`:
- Around line 3044-3045: The CmuxDogfoodFeedbackSink package is registered as a
local package reference and linked in the Frameworks section, but the cmux
target's packageProductDependencies block is missing the corresponding
dependency entry for identifier DF60000000000000000000F2. Add this package
product dependency identifier to the cmux target's packageProductDependencies
array to complete the target-level wiring and ensure Xcode can properly resolve
and maintain the package dependency during project regeneration.
In
`@Packages/CmuxDogfoodFeedbackSink/Sources/CmuxDogfoodFeedbackSink/DogfoodFeedbackLimits.swift`:
- Around line 34-48: The DogfoodFeedbackLimits.init method accepts all numeric
parameters without validation, but these values are used directly in
String.prefix() and dropLast() operations which require non-negative arguments
and will crash with precondition failures if negative values are provided. Add
guard statements in the initializer to validate that maxTextChars,
maxTerminalChars, maxBuildStampChars, maxBlobBase64Chars, maxBlobBytes, and
maxRetainedBundles are all non-negative, raising a precondition failure with a
descriptive error message if any value is negative.
In
`@Packages/CmuxDogfoodFeedbackSink/Tests/CmuxDogfoodFeedbackSinkTests/DogfoodFeedbackServiceTests.swift`:
- Around line 38-65: Add no-side-effects assertions to both the
base64CharCapRejected() and blobByteCapRejected() test methods to verify that
rejected submissions do not create any files. After each service.submit() call,
add an assertion similar to what exists in the unauthorized test that checks
FileManager.default.fileExists(atPath: root.path) returns false. This ensures
that the base64/blob validation checks continue to occur before any directory
creation, and will catch future refactors that inadvertently move I/O operations
earlier in the validation pipeline.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 41e38a07-788d-44b6-b3ee-051c4ef6c644
⛔ Files ignored due to path filters (1)
.github/swift-file-length-budget.tsvis excluded by!**/*.tsv
📒 Files selected for processing (8)
Packages/CmuxDogfoodFeedbackSink/Package.swiftPackages/CmuxDogfoodFeedbackSink/Sources/CmuxDogfoodFeedbackSink/DogfoodFeedbackLimits.swiftPackages/CmuxDogfoodFeedbackSink/Sources/CmuxDogfoodFeedbackSink/DogfoodFeedbackOutcome.swiftPackages/CmuxDogfoodFeedbackSink/Sources/CmuxDogfoodFeedbackSink/DogfoodFeedbackService.swiftPackages/CmuxDogfoodFeedbackSink/Sources/CmuxDogfoodFeedbackSink/DogfoodFeedbackSubmission.swiftPackages/CmuxDogfoodFeedbackSink/Tests/CmuxDogfoodFeedbackSinkTests/DogfoodFeedbackServiceTests.swiftSources/TerminalController.swiftcmux.xcodeproj/project.pbxproj
👮 Files not reviewed due to content moderation or server errors (1)
- Sources/TerminalController.swift
Comment on lines
+3044
to
3045
| DF60000000000000000000F1 /* XCLocalSwiftPackageReference "CmuxDogfoodFeedbackSink" */, | ||
| C9A2D00000000000000000D1 /* XCLocalSwiftPackageReference "CmuxFeedbackUI" */, |
There was a problem hiding this comment.
Missing target-level package dependency registration for CmuxDogfoodFeedbackSink.
CmuxDogfoodFeedbackSink is added as a local package + product (Lines 3044 and 4884), and linked in Frameworks (Line 1779), but the cmux target’s packageProductDependencies block does not include DF60000000000000000000F2. This incomplete wiring can break package resolution/link stability in Xcode project regeneration.
Suggested pbxproj fix
A5001050 /* cmux */ = {
isa = PBXNativeTarget;
@@
packageProductDependencies = (
@@
A8BD195031FC4B82B4354297 /* StackAuth */,
EFB18E3B3099DFE2ECA3C263 /* CMUXMobileCore */,
+ DF60000000000000000000F2 /* CmuxDogfoodFeedbackSink */,
);As per coding guidelines, this is a project wiring completeness issue in cmux.xcodeproj/** that should be validated at target level, not only by presence of package reference/product objects.
Also applies to: 4884-4888
🤖 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 `@cmux.xcodeproj/project.pbxproj` around lines 3044 - 3045, The
CmuxDogfoodFeedbackSink package is registered as a local package reference and
linked in the Frameworks section, but the cmux target's
packageProductDependencies block is missing the corresponding dependency entry
for identifier DF60000000000000000000F2. Add this package product dependency
identifier to the cmux target's packageProductDependencies array to complete the
target-level wiring and ensure Xcode can properly resolve and maintain the
package dependency during project regeneration.
Source: Coding guidelines
Comment on lines
+34
to
+48
| public init( | ||
| maxTextChars: Int, | ||
| maxTerminalChars: Int, | ||
| maxBuildStampChars: Int, | ||
| maxBlobBase64Chars: Int, | ||
| maxBlobBytes: Int, | ||
| maxRetainedBundles: Int | ||
| ) { | ||
| self.maxTextChars = maxTextChars | ||
| self.maxTerminalChars = maxTerminalChars | ||
| self.maxBuildStampChars = maxBuildStampChars | ||
| self.maxBlobBase64Chars = maxBlobBase64Chars | ||
| self.maxBlobBytes = maxBlobBytes | ||
| self.maxRetainedBundles = maxRetainedBundles | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify the unchecked public initializer and downstream use-sites of limits.
set -euo pipefail
echo "== DogfoodFeedbackLimits initializer (look for missing preconditions) =="
rg -n -C3 'public init\(|self\.max(TextChars|TerminalChars|BuildStampChars|maxBlobBase64Chars|maxBlobBytes|maxRetainedBundles)' \
Packages/CmuxDogfoodFeedbackSink/Sources/CmuxDogfoodFeedbackSink/DogfoodFeedbackLimits.swift
echo
echo "== Downstream consumers that assume non-negative values =="
rg -n -C3 'prefix\(limits\.|maxRetainedBundles|maxBlobBytes|maxBlobBase64Chars' \
Packages/CmuxDogfoodFeedbackSink/Sources/CmuxDogfoodFeedbackSink/DogfoodFeedbackService.swiftRepository: manaflow-ai/cmux
Length of output: 3259
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Custom Swift lint rules ==="
fd -type f -name '*.md' -o -name '*.swift' .github/review-bot-rules/ 2>/dev/null | head -20
echo
echo "== Full DogfoodFeedbackLimits.swift to check defaults and factory methods ==="
cat -n Packages/CmuxDogfoodFeedbackSink/Sources/CmuxDogfoodFeedbackSink/DogfoodFeedbackLimits.swift
echo
echo "== pruneBundles implementation to check negative handling ==="
rg -n -B2 -A15 'func pruneBundles' Packages/CmuxDogfoodFeedbackSink/Sources/CmuxDogfoodFeedbackSink/DogfoodFeedbackService.swiftRepository: manaflow-ai/cmux
Length of output: 91
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Check for review-bot-rules or similar directories ==="
find . -type d -name '*review*' -o -name '*rules*' 2>/dev/null | grep -v node_modules | head -20
echo
echo "== Full DogfoodFeedbackLimits.swift ==="
wc -l Packages/CmuxDogfoodFeedbackSink/Sources/CmuxDogfoodFeedbackSink/DogfoodFeedbackLimits.swift
echo
echo "== Content ==="
cat -n Packages/CmuxDogfoodFeedbackSink/Sources/CmuxDogfoodFeedbackSink/DogfoodFeedbackLimits.swift
echo
echo "== pruneBundles implementation ==="
rg -n -B2 -A20 'func pruneBundles' Packages/CmuxDogfoodFeedbackSink/Sources/CmuxDogfoodFeedbackSink/DogfoodFeedbackService.swiftRepository: manaflow-ai/cmux
Length of output: 4514
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Check .github/review-bot-rules for Swift rules ==="
ls -la .github/review-bot-rules/ 2>/dev/null || echo "Directory not found"
echo
echo "== Verify String.prefix behavior with negative count (using python) ==="
python3 <<'PY'
# Swift's String.prefix with negative count should trigger a precondition failure.
# Let's check the Swift documentation and standard library behavior.
# According to Swift stdlib:
# - String.prefix(_:) requires count >= 0
# - Sequence.dropLast(_:) requires the argument >= 0
# Both are preconditions that will trap if violated.
print("String.prefix behavior:")
print("- Takes a non-negative Int as argument")
print("- Precondition: count >= 0")
print("- If count < 0: crashes with precondition failure")
print()
print("Sequence.dropLast behavior:")
print("- Takes a non-negative Int as argument")
print("- Precondition: n >= 0")
print("- If n < 0: crashes with precondition failure")
print()
print("Impact in DogfoodFeedbackService.submit:")
print("- Line 93-95 use String.prefix(limits.max*) without validation")
print("- Line 211 uses dropLast(keep) where keep = maxRetainedBundles")
print("- Both will crash if negative limits are passed")
PY
echo
echo "== Check if DogfoodFeedbackLimits is used elsewhere ==="
rg -l 'DogfoodFeedbackLimits' --type swift | head -20
echo
echo "== Check all call sites of DogfoodFeedbackLimits init ==="
rg -n 'DogfoodFeedbackLimits(' --type swift -A2 -B2Repository: manaflow-ai/cmux
Length of output: 2612
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Check all instantiation call sites of DogfoodFeedbackLimits ==="
rg -n 'DogfoodFeedbackLimits' --type swift -A3 -B1 | grep -E '(init|\.default)'
echo
echo "== Check tests for DogfoodFeedbackLimits ==="
cat -n Packages/CmuxDogfoodFeedbackSink/Tests/CmuxDogfoodFeedbackSinkTests/DogfoodFeedbackServiceTests.swift | head -100
echo
echo "== Check for any guards or preconditions in DogfoodFeedbackService on limits ==="
rg -n 'guard|precondition' Packages/CmuxDogfoodFeedbackSink/Sources/CmuxDogfoodFeedbackSink/DogfoodFeedbackService.swiftRepository: manaflow-ai/cmux
Length of output: 7274
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Verify that dropLast in pruneBundles will crash with negative keep ==="
cat -n Packages/CmuxDogfoodFeedbackSink/Sources/CmuxDogfoodFeedbackSink/DogfoodFeedbackService.swift | sed -n '201,214p'
echo
echo "== Check if there are any other entry points that could construct DogfoodFeedbackLimits with user input ==="
rg -n 'DogfoodFeedbackLimits\|limits\s*=' Packages/CmuxDogfoodFeedbackSink/Sources/ -A2 -B2
echo
echo "== Verify: does pruneBundles handle the case where keep is negative? ==="
echo "Line 210: guard directories.count > keep else { return }"
echo " - This check passes if directories.count <= keep"
echo " - Line 211: dropLast(keep) will crash if keep < 0"
echo " - This is a precondition violation in Swift stdlib"Repository: manaflow-ai/cmux
Length of output: 1013
Enforce non-negative invariants in DogfoodFeedbackLimits.init.
DogfoodFeedbackService.submit consumes these values directly in String.prefix(limits.max*Chars) (lines 93–95) and directories.dropLast(keep) (line 211), both of which require non-negative arguments and will crash with a precondition failure if negative limits are passed. Add guards to validate at construction time.
Suggested fix
public init(
maxTextChars: Int,
maxTerminalChars: Int,
maxBuildStampChars: Int,
maxBlobBase64Chars: Int,
maxBlobBytes: Int,
maxRetainedBundles: Int
) {
+ precondition(maxTextChars >= 0, "maxTextChars must be >= 0")
+ precondition(maxTerminalChars >= 0, "maxTerminalChars must be >= 0")
+ precondition(maxBuildStampChars >= 0, "maxBuildStampChars must be >= 0")
+ precondition(maxBlobBase64Chars >= 0, "maxBlobBase64Chars must be >= 0")
+ precondition(maxBlobBytes >= 0, "maxBlobBytes must be >= 0")
+ precondition(maxRetainedBundles >= 0, "maxRetainedBundles must be >= 0")
self.maxTextChars = maxTextChars
self.maxTerminalChars = maxTerminalChars
self.maxBuildStampChars = maxBuildStampChars
self.maxBlobBase64Chars = maxBlobBase64Chars
self.maxBlobBytes = maxBlobBytes
self.maxRetainedBundles = maxRetainedBundles
}🤖 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
`@Packages/CmuxDogfoodFeedbackSink/Sources/CmuxDogfoodFeedbackSink/DogfoodFeedbackLimits.swift`
around lines 34 - 48, The DogfoodFeedbackLimits.init method accepts all numeric
parameters without validation, but these values are used directly in
String.prefix() and dropLast() operations which require non-negative arguments
and will crash with precondition failures if negative values are provided. Add
guard statements in the initializer to validate that maxTextChars,
maxTerminalChars, maxBuildStampChars, maxBlobBase64Chars, maxBlobBytes, and
maxRetainedBundles are all non-negative, raising a precondition failure with a
descriptive error message if any value is negative.
Comment on lines
+38
to
+65
| @Test("oversized base64 string is rejected without decoding") | ||
| func base64CharCapRejected() async throws { | ||
| let limits = DogfoodFeedbackLimits( | ||
| maxTextChars: 16, maxTerminalChars: 16, maxBuildStampChars: 16, | ||
| maxBlobBase64Chars: 4, maxBlobBytes: 1024, maxRetainedBundles: 5 | ||
| ) | ||
| let (service, _) = makeService(limits: limits) | ||
| let outcome = await service.submit( | ||
| DogfoodFeedbackSubmission(text: "", terminalText: "", buildStamp: "", diagnosticBlobBase64: "AAAAAAAA"), | ||
| authenticatedEmail: "a@manaflow.ai" | ||
| ) | ||
| #expect(outcome == .invalidParams(reason: "diagnostic_blob_base64 exceeds size limit")) | ||
| } | ||
|
|
||
| @Test("decoded blob over the byte cap is dropped") | ||
| func blobByteCapRejected() async throws { | ||
| let limits = DogfoodFeedbackLimits( | ||
| maxTextChars: 16, maxTerminalChars: 16, maxBuildStampChars: 16, | ||
| maxBlobBase64Chars: 1_000_000, maxBlobBytes: 4, maxRetainedBundles: 5 | ||
| ) | ||
| let (service, _) = makeService(limits: limits) | ||
| let blob = Data(repeating: 0xAB, count: 32).base64EncodedString() | ||
| let outcome = await service.submit( | ||
| DogfoodFeedbackSubmission(text: "", terminalText: "", buildStamp: "", diagnosticBlobBase64: blob), | ||
| authenticatedEmail: "a@manaflow.ai" | ||
| ) | ||
| #expect(outcome == .invalidParams(reason: "diagnostic blob exceeds size limit")) | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
Consider adding no-I/O assertions for consistency with unauthorized.
The unauthorized test asserts !FileManager.default.fileExists(atPath: root.path) to verify no side effects on rejection. These rejection tests rely on the implementation detail that base64/blob cap checks occur before directory creation. Adding the same assertion would catch future refactors that inadvertently move I/O earlier in the validation path.
💡 Optional: Add defensive no-side-effects assertions
`@Test`("oversized base64 string is rejected without decoding")
func base64CharCapRejected() async throws {
let limits = DogfoodFeedbackLimits(
maxTextChars: 16, maxTerminalChars: 16, maxBuildStampChars: 16,
maxBlobBase64Chars: 4, maxBlobBytes: 1024, maxRetainedBundles: 5
)
- let (service, _) = makeService(limits: limits)
+ let (service, root) = makeService(limits: limits)
let outcome = await service.submit(
DogfoodFeedbackSubmission(text: "", terminalText: "", buildStamp: "", diagnosticBlobBase64: "AAAAAAAA"),
authenticatedEmail: "a@manaflow.ai"
)
`#expect`(outcome == .invalidParams(reason: "diagnostic_blob_base64 exceeds size limit"))
+ `#expect`(!FileManager.default.fileExists(atPath: root.path))
}
`@Test`("decoded blob over the byte cap is dropped")
func blobByteCapRejected() async throws {
let limits = DogfoodFeedbackLimits(
maxTextChars: 16, maxTerminalChars: 16, maxBuildStampChars: 16,
maxBlobBase64Chars: 1_000_000, maxBlobBytes: 4, maxRetainedBundles: 5
)
- let (service, _) = makeService(limits: limits)
+ let (service, root) = makeService(limits: limits)
let blob = Data(repeating: 0xAB, count: 32).base64EncodedString()
let outcome = await service.submit(
DogfoodFeedbackSubmission(text: "", terminalText: "", buildStamp: "", diagnosticBlobBase64: blob),
authenticatedEmail: "a@manaflow.ai"
)
`#expect`(outcome == .invalidParams(reason: "diagnostic blob exceeds size limit"))
+ `#expect`(!FileManager.default.fileExists(atPath: root.path))
}🤖 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
`@Packages/CmuxDogfoodFeedbackSink/Tests/CmuxDogfoodFeedbackSinkTests/DogfoodFeedbackServiceTests.swift`
around lines 38 - 65, Add no-side-effects assertions to both the
base64CharCapRejected() and blobByteCapRejected() test methods to verify that
rejected submissions do not create any files. After each service.submit() call,
add an assertion similar to what exists in the unauthorized test that checks
FileManager.default.fileExists(atPath: root.path) returns false. This ensures
that the base64/blob validation checks continue to occur before any directory
creation, and will catch future refactors that inadvertently move I/O operations
earlier in the validation pipeline.
Collaborator
Author
|
Superseded by #6224 — consolidated into the existing CmuxFeedback/CmuxAppKitSupportUI package (no new micro-package) per over-engineering review. The extraction is preserved there. |
This branch was successfully deployed
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.
Extracts the privileged Mac<->phone dogfood feedback persistence domain out of the
TerminalControllergod file into a new leaf package,CmuxDogfoodFeedbackSink(zero deps, manifest mirrorsCmuxMobileSupport).What moved
DogfoodFeedbackLimits— the field/blob size caps and bundle-retention cap as aSendablevalue type..defaultcarries the exact prior static constants (16384 text, 262144 terminal, 512 build stamp, ~8 MiB base64 / 6 MiB decoded, keep 50).DogfoodFeedbackService.isPrivilegedFeedbackEmail— the@manaflow.aidomain gate (trim + lowercase + suffix), moved verbatim.DogfoodFeedbackService.submit— anonisolatedSendable Service that does the cheap caller-actor field caps, rejects an oversized base64 string without decoding, then runs the decode + bundle write + prune on a detached utility task.DogfoodFeedbackOutcome— one case per RPC response (written/unauthorized/invalidParams(reason)/internalError), replacing the old internalDogfoodFeedbackWriteOutcome.DogfoodFeedbackSubmission— the four raw wire fields as aSendablevalue.Seams (constructor-injected, inverting the god-type reach)
FileManagervia an@Sendable () -> FileManagerprovider (FileManager isn'tSendableand the writer runs on a detached task), the cache-rootURL, and a@Sendable () -> Dateclock. All default to the production values (~/.cache/cmux-dogfood-feedback,FileManager.default, current date), so tests drive it against a temp dir with a fixed timestamp.App side (thin forward)
v2MobileDogfoodFeedbackSubmitnow resolves the authenticated email via the main-actorMobileHostService.shared, reads the four wire fields viav2RawString, callsDogfoodFeedbackService().submit(...), and maps the outcome toV2CallResult. The service re-enforces the privilege gate at the boundary.Byte-identical
Same caps, same base64-char-cap-before-decode then decoded-byte-cap ordering, same
Task.detached(priority: .utility)off-main structure, same bundle dir naming (ISO8601 internet datetime, colons ->-, 8-char lowercase UUID), same 0700 dir / 0600 file perms created dir-first, samebundle.jsonschema/keys/sortedKeys/prettyPrinted, same lexicographic prune keeping the newest 50, and the same RPC errorcode/message/dataplus.okpayload keys (ok,bundle_path,diagnostic_log_bytes).Tests
7 behavior tests (
swift testgreen): privilege gate normalization, unauthorized-before-any-IO, base64 char cap rejection, decoded byte-cap rejection, bundle write + manifest contents + 0700/0600 perms, field capping, and prune-to-N. Fakes the seams with an injected temp dir + fixed clock.Stats
TerminalController.swift(14785 -> 14638); file-length budget ratcheted down.cmux.xcodeproj(5 pbxproj entries mirroringCmuxFeedback, pbxproj normalized) and the app target import.NOTE: full app build + tests validated by GitHub CI after push (not run locally). Do not merge.
🤖 Generated with Claude Code
Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Summary by cubic
Extracted the privileged dogfood feedback sink from
TerminalControllerinto a new leaf package,CmuxDogfoodFeedbackSink, keeping behavior the same and shrinking the controller. The RPC now forwards toDogfoodFeedbackServicefor validation and disk writes.CmuxDogfoodFeedbackSink(zero deps) withDogfoodFeedbackService,DogfoodFeedbackLimits,DogfoodFeedbackOutcome,DogfoodFeedbackSubmission, and movedisPrivilegedFeedbackEmail.v2MobileDogfoodFeedbackSubmitnow resolves the authenticated email viaMobileHostService.shared, reads wire fields, callsDogfoodFeedbackService().submit(...), and maps the outcome toV2CallResult.cmux.xcodeproj; removed ~147 LOC fromTerminalController.swiftand updated the file-length budget. Added 7 behavior tests covering the gate, caps, write/manifest/perms, and pruning.Written for commit 33c7c41. Summary will update on new commits.
Summary by CodeRabbit
Release Notes
New Features
Refactor
Tests