fix(testkit): bound probe output

[VailElla]

Summary

• cap retained probe stdout/stderr at 4 MiB with incremental reads
• terminate and reject over-limit probes; never upload truncated output
• cap encoded JSON request bodies at 8 MiB
• add boundary, unbounded-child, timeout-compatibility, and JSON-size tests

Root cause

TestKitRunner used readDataToEndOfFile(), retained all probe output, then copied it into a String and JSON request body. The 30-second watchdog bounded runtime but not memory usage.

Security invariant

No probe can cause the parent to retain more than 4 MiB of output or send more than an 8 MiB encoded request body. Oversized output is rejected rather than truncated and uploaded.

Validation

• focused Xcode-toolchain test suite: 7 passed; after review fixes, the suite passed 3 consecutive runs (21/21)
• release build for WhatCable: passed
• release build for whatcable-cli: passed
• repository-wide suite attempted: the candidate tests pass; unrelated corpus suites report 76 fixture issues because research/customer-probes and research/dumps are absent from this public mirror

Review

• full-file security diff review completed for both changed files
• incremental reviews completed for the test-stabilization and automated-review response commits
• addressed read-error process cleanup and incomplete UTF-8 partial-output handling raised during automated review
• no reportable security regressions or remaining candidate-introduced bugs found

[coderabbitai]

📝 Walkthrough

Walkthrough

TestKitRunner now limits captured probe output and serialized JSON request bodies. Probe execution reports output-limit violations, terminates oversized processes, and preserves eligible timeout output. New tests cover exact-limit, oversized, timeout, termination, and request-body cases.

Changes

TestKitRunner size limits

Layer / File(s)	Summary
Bounded probe execution Sources/WhatCable/Services/TestKitRunner.swift, Tests/WhatCableDarwinTests/TestKitRunnerOutputLimitTests.swift	Probe output is read in chunks up to maxProbeOutputBytes; oversized probes are terminated and reported through didExceedOutputLimit, while timeout-path output remains available. Tests cover output boundaries, termination, and partial output.
Bounded JSON submission Sources/WhatCable/Services/TestKitRunner.swift, Tests/WhatCableDarwinTests/TestKitRunnerOutputLimitTests.swift	boundedJSONBody rejects serialized payloads exceeding maxRequestBodyBytes, and postJSON skips those requests. Tests cover oversized and accepted payloads.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Sequence Diagram(s)

sequenceDiagram
  participant TestKitRunner
  participant ProbeProcess
  participant StdoutPipe
  TestKitRunner->>ProbeProcess: Launch probe
  ProbeProcess->>StdoutPipe: Produce stdout chunks
  TestKitRunner->>StdoutPipe: Read bounded chunks
  TestKitRunner->>ProbeProcess: Terminate on output overflow
  TestKitRunner-->>TestKitRunner: Return ProbeRunResult

Loading

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title is concise and accurately highlights the main change: bounding TestKit probe output.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (1)

Sources/WhatCable/Services/TestKitRunner.swift (1)

290-294: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low value

Terminate the process immediately on read failure to avoid a timeout-driven delay.

If pipe.fileHandleForReading.read throws an error, the while loop exits, but the process may still be running. It will likely block once its stdout pipe buffer fills, causing process.waitUntilExit() to hang until the 30-second watchdog timer finally fires. Terminating the process here prevents this unnecessary delay.

♻️ Proposed refactor

                 } catch {
                     didFailReading = true
                     Self.log.error("Failed to read probe output: \(error.localizedDescription)")
+                    if process.isRunning {
+                        process.terminate()
+                    }
                 }
                 process.waitUntilExit()

🤖 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 `@Sources/WhatCable/Services/TestKitRunner.swift` around lines 290 - 294,
Update the read-error catch block in the probe-output loop to terminate the
running process immediately after setting didFailReading, before
process.waitUntilExit() is reached. Use the existing process instance and
preserve the current error logging and failure-state handling.

🤖 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 `@Sources/WhatCable/Services/TestKitRunner.swift`:
- Around line 295-298: Update the output conversion in TestKitRunner’s probe
result construction to use resilient UTF-8 decoding via String(decoding:as:)
instead of String(data:encoding:), while preserving the existing nil behavior
when didExceedOutputLimit or didFailReading is true. This must retain partial
output and replace any incomplete UTF-8 bytes rather than discarding the output.

---

Nitpick comments:
In `@Sources/WhatCable/Services/TestKitRunner.swift`:
- Around line 290-294: Update the read-error catch block in the probe-output
loop to terminate the running process immediately after setting didFailReading,
before process.waitUntilExit() is reached. Use the existing process instance and
preserve the current error logging and failure-state handling.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 5278953b-e838-4c2a-b700-0c8beacad29a

📥 Commits

Reviewing files that changed from the base of the PR and between 0dcfc8a and be5e1bb.

📒 Files selected for processing (2)

• Sources/WhatCable/Services/TestKitRunner.swift
• Tests/WhatCableDarwinTests/TestKitRunnerOutputLimitTests.swift

[darrylmorley]

Thanks @VailElla, this was a well-put-together PR. The unbounded readDataToEndOfFile() was a real hole, and the reject-don't-truncate call was the right one: a truncated dump is worse than no dump for the analysis we do on these.

Merged with a few adaptations on the way in. This repo is a mirror of a private development repo, so your change lands on main via a mirror commit rather than the squash directly, and the test file moved to a different test target to match the internal layout. While reviewing we also added SIGTERM-to-SIGKILL escalation (a probe that ignores SIGTERM could previously hold the runner hostage even with your cap in place), a per-chunk autorelease pool in the drain loop, and timing assertions on the watchdog tests.

Ships in the next release.

[VailElla]

Thanks for the thoughtful review and for merging this! The additional hardening makes a lot of sense. Glad to see it shipping in the next release!