test: Provider-Level Log-Stop Failure Test Coverage

[droid-ash]

Meta

Change ID	Type	Confidence	Plan	Review
cjx8	test	4.7/5.0	5/5 tasks, 11/11 acceptance ✓	✓ 1 cycle

Impact	+/−	Net
true	+456 / −1	+455
└ impl	+390 / −1	+389
└ tests	+66 / −0	+66

_{generated by fab-kit v2.16.8}

Pipeline: intake ✓ → apply ✓ → review ✓ → hydrate ✓ → ship → review-pr

Summary

PR #168 fixed a defect where stopLogCapture reported success over a log file that was never written, but no test ever drove a failing write stream through a provider's stopLogCapture — all prior tests either hit LogWriteStreamRegistry directly or used a good writable path. An adversarial mutation review proved this: swapping finalize for finalizeQuietly at the providers' success-path call sites restores the original defect and all 120 tests stay green. This change adds mutation-killing provider-level tests and documents two related behavior decisions without any source behavior change.

Changes

• New provider-level failing-stream stop tests (Android + iOS), asserting success: false, an ENOENT-matching message, and zero live streams after stop — appended before the deliberately-last throwing-sink test, killing the finalize/finalizeQuietly mutation
• Mutation-kill verification performed procedurally (mutation applied, confirmed new tests fail, reverted via git checkout, confirmed clean and green) — no committed artifact
• Assessment item A: keep finalize's unconditional recorded-error rejection (no writableFinished guard) — documented via rationale comment; the contradicting state is unreachable in production because autoDestroy means every real error destroys the stream
• Assessment item B: document the once-consumed recorded-error ordering invariant (quiet-then-loud) via code comment and memory update — no redesign, no test pinning the accepted hazard
• docs/memory/device-node/log-capture.md updated with the new test surface and both Design Decisions

Summary by CodeRabbit

• Bug Fixes

  • Log capture stops now report failures when the output stream cannot be opened.
  • Original stream errors, including ENOENT, are propagated even after a successful flush.
  • Failed stops clean up tracked capture entries reliably.

• Tests

  • Added Android and iOS coverage for asynchronous log-stream opening failures.
  • Added verification for failure responses, error propagation, and cleanup behavior.

• Documentation

  • Documented log finalization error handling and cleanup ordering.

[coderabbitai]

Warning

Review limit reached

@droid-ash, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 7 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: b75f4652-b3f6-410f-9d72-16ac9b64df80

📥 Commits

Reviewing files that changed from the base of the PR and between 2a571a7 and 8e0cbcd.

📒 Files selected for processing (3)

• fab/changes/260731-cjx8-provider-log-stop-test-coverage/.history.jsonl
• fab/changes/260731-cjx8-provider-log-stop-test-coverage/.status.yaml
• packages/device-node/src/device/logWriteStream.ts

📝 Walkthrough

Walkthrough

The change documents log-stream finalization error behavior and adds Android and iOS provider tests for asynchronous output-stream open failures. The tests verify ENOENT reporting, unsuccessful stop responses, error propagation, and registry cleanup.

Changes

Provider log-stop failure coverage

Layer / File(s)	Summary
Test contract and acceptance criteria fab/changes/260731-cjx8-provider-log-stop-test-coverage/*	Defines provider failure scenarios, deterministic synchronization, acceptance criteria, verification steps, and workflow status.
Finalization error semantics packages/device-node/src/device/logWriteStream.ts, docs/memory/device-node/log-capture.md	Documents recorded-error propagation through finalize, error consumption during cleanup, and the required finalize before finalizeQuietly ordering.
Cross-platform provider failure tests packages/device-node/src/device/test/logCaptureProviders.test.ts, docs/memory/device-node/log-capture.md	Adds Android and iOS tests for failed stream opens. The tests verify success: false, ENOENT, error propagation, and registry removal.

Estimated code review effort: 2 (Simple) | ~15 minutes

Possibly related PRs

• droid-ash/finalrun-agent#69: Introduces the log-capture behavior extended by these provider stop-failure tests.
• droid-ash/finalrun-agent#160: Modifies the same device-node log-capture area.
• droid-ash/finalrun-agent#161: Covers related log-capture lifecycle and stream cleanup behavior.

Suggested labels: enhancement

Suggested reviewers: arnoldlaishram

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and concisely describes the provider-level log-stop failure test coverage added by the pull request.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
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.

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch 260731-cjx8-provider-log-stop-test-coverage

---

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

[coderabbitai]

Actionable comments posted: 2

🧹 Nitpick comments (1)

packages/device-node/src/device/test/logCaptureProviders.test.ts (1)

241-251: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Share the provider factory between platform loops.

The new loop duplicates the createProvider factory from Lines 121-130. Extract one helper that accepts platform and FakeChildProcess, then use it in both loops. This prevents Android and iOS test setup from diverging.

🤖 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/device-node/src/device/test/logCaptureProviders.test.ts` around
lines 241 - 251, Extract the duplicated createProvider factory into a shared
helper that accepts a platform and FakeChildProcess and returns the
corresponding AndroidLogcatProvider or IOSLogProvider. Replace the local factory
definitions in both platform loops with this helper, preserving the existing
execFileStub and spawnFn setup.

🤖 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 `@packages/device-node/src/device/logWriteStream.ts`:
- Around line 191-202: Serialize concurrent stop operations in LogCaptureManager
before awaiting either provider, using a per-capture single-flight guard or
claiming the capture in _stoppedTestCases before the provider call so only one
stop can reach finalization. Preserve the required loud-before-quiet behavior
and update the comments in packages/device-node/src/device/logWriteStream.ts
(lines 191-202) and docs/memory/device-node/log-capture.md (lines 331-350) to
describe the enforced serialization and resulting ownership semantics.
- Around line 136-140: The finalize flow in logWriteStream.ts can report success
before asynchronous close-time errors are recorded. Update the writableFinished
path in finalize to await the stream’s terminal close/error state before
evaluating entry.error, add a regression test covering an error emitted during
auto-destroy after writableFinished, and update
docs/memory/device-node/log-capture.md lines 303-321 to document that successful
stop waits for close-time errors.

---

Nitpick comments:
In `@packages/device-node/src/device/test/logCaptureProviders.test.ts`:
- Around line 241-251: Extract the duplicated createProvider factory into a
shared helper that accepts a platform and FakeChildProcess and returns the
corresponding AndroidLogcatProvider or IOSLogProvider. Replace the local factory
definitions in both platform loops with this helper, preserving the existing
execFileStub and spawnFn setup.

🪄 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: CHILL

Plan: Pro Plus

Run ID: 01208b2a-49dc-4f21-8563-12e56bb5c55d

📥 Commits

Reviewing files that changed from the base of the PR and between 7fdebd0 and 2a571a7.

📒 Files selected for processing (7)

• docs/memory/device-node/log-capture.md
• fab/changes/260731-cjx8-provider-log-stop-test-coverage/.history.jsonl
• fab/changes/260731-cjx8-provider-log-stop-test-coverage/.status.yaml
• fab/changes/260731-cjx8-provider-log-stop-test-coverage/intake.md
• fab/changes/260731-cjx8-provider-log-stop-test-coverage/plan.md
• packages/device-node/src/device/logWriteStream.ts
• packages/device-node/src/device/test/logCaptureProviders.test.ts