[droid-ash]

Meta

Change ID	Type	Confidence	Plan	Review
eyvt	ci	4.6/5.0	8/8 tasks, 23/23 acceptance ✓	✓ 2 cycles

Impact	+/−	Net
true	+1447 / −42	+1405
└ impl	+1272 / −41	+1231
└ tests	+175 / −1	+174

_{generated by fab-kit v2.16.8}

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

Summary

The ios driver-CI job is the most expensive thing in this repo's CI: macos-latest bills at 10× the Linux rate, drivers.yml runs on every PR regardless of relevance, and superseded pushes are paid for in full (PR #167's three pushes cost ~200 billable minutes for one informative verdict). This change adds two cost guards to .github/workflows/drivers.yml — job-level timeout-minutes and a paths filter plus PR-scoped concurrency block — and separately fixes a carried-forward defect in packages/device-node's log write-stream registry, where a missing persistent error listener could either crash the CLI mid-run or silently report a successful stop over a log file that was never written.

Changes

1. timeout-minutes on both drivers.yml jobs — 15 (android) / 25 (ios), set at job level (not step level, so checkout and toolchain setup are inside the bound). No workflow in the repo previously set one, so every job sat on GitHub's 360-minute default; at the 10× macos-latest rate a wedged xcodebuild could bill ~3,600 minutes. Both values are ≥2× the worst of five observed runs (android 2m36s, ios 9m20s worst-case), with ios deliberately the tighter relative bound because its minutes cost 10×. Worst-case ios exposure drops 3,600 → 250 billable minutes.

2. A paths: filter on both triggers, and a PR-scoped concurrency block. The filter — drivers/**, proto/**, both scripts/build-drivers-*.sh, and the workflow file itself — is byte-identical across pull_request and push. proto/** is load-bearing: drivers/android/app/build.gradle.kts:104 adds the repo-root proto dir as a protobuf source dir, so a proto-only edit can break the Android Kotlin compile with no file under drivers/ changing. The concurrency group is drivers-${{ github.event_name == 'pull_request' && github.ref || github.run_id }} with cancel-in-progress gated to pull_request: superseded PR pushes cancel in flight, while main runs stay run-id-scoped so one is never queued behind or evicted by another — a bare group: ${{ github.ref }} would reintroduce the pending-eviction footgun this repo already documents for ci.yml. This is safe here specifically because ruleset 14531661 requires only ci.yml's test context, so no merge gate consults drivers.yml. ci.yml itself is deliberately untouched — its no-concurrency rule stays correct for the required check.

3. logWriteStream.ts: a persistent error listener in LogWriteStreamRegistry.open(). open() previously returned a bare fs.createWriteStream, and pipe() does not forward destination errors to the source, so the first EACCES/ENOSPC surfaced as an unhandled 'error' event — which Node throws, killing the CLI mid-run. The defect had a second face: _endAndFlush early-returns on an already-destroyed stream, and an errored stream auto-destroys, so an error arriving after finalize ran meant the stop resolved as success over a log file that was never actually written. open() now attaches a persistent listener that records the first error on a per-path entry, and finalize re-throws that recorded error after its existing try/finally (so an in-flight drain or flush rejection keeps precedence and is never masked). Both that listener's Logger.e call and finalizeQuietly's are wrapped in try/catch, because Logger._emit's sink loop is itself unguarded and the CLI's runner-log sink is a bare fs.appendFileSync — a listener of last resort must not throw. Six new registry test cases were added, all verified to fail against the pre-fix code.

Scope note. A fourth item from the original request — adding .github/ to the /git-pr skill's expected write areas — was deliberately dropped and is tracked separately as backlog ax7i, to be fixed upstream in fab-kit. .claude/skills/git-pr/SKILL.md is gitignored and kit-deployed, so any edit here would be outside version control, invisible in this diff, and lost on the next fab-kit upgrade. Nothing under .claude/ or ~/.fab-kit/ is touched by this PR.

Verification.

• npm run build, npm run typecheck, npm run lint all clean (lint has 42 pre-existing warnings, none in touched files).
• Full suite green: 545 tests across workspaces, packages/device-node 120/120.
• drivers.yml parses; both paths lists are byte-identical; both concurrency expressions verified for pull_request and push.
• Honest caveat: the paths filter's exclusion behavior is not proven by this PR — this PR edits .github/workflows/drivers.yml, which is itself in the filter, so the drivers workflow will fire on it. That is the filter working, not a failure. The concurrency block is only exercised by two pushes in quick succession, so it ships source-verified unless this PR happens to receive them.

Summary by CodeRabbit

• Bug Fixes

  • Device log capture now records the first underlying write-stream error and surfaces it when capture stops, preventing silent failures and preserving correct behavior for quiet cleanup.

• CI Improvements

  • Native driver checks are now path-filtered, bounded by explicit timeouts, and cancel superseded pull-request runs while keeping main runs uninterrupted.

• Documentation

  • Updated CI quality-gate and native driver compile/verification descriptions, plus expanded log-capture error-handling requirements to match the new runtime behavior.

[coderabbitai]

📝 Walkthrough

Walkthrough

The PR adds path-filtered, timeout-bounded, concurrency-controlled native driver workflows and updates their CI documentation. It also records write-stream errors for later finalization failure, hardens quiet logging, expands regression tests, and adds Fab planning and execution records.

Changes

CI and device reliability

Layer / File(s)	Summary
Driver workflow controls .github/workflows/drivers.yml, docs/memory/ci/*, docs/memory/drivers/grpc-contract.md, docs/memory/index.md	Driver builds now use relevant-path triggers, PR cancellation groups, per-run main grouping, and Android/iOS timeout limits. CI documentation describes the updated gate semantics.
Log stream error finalization packages/device-node/src/device/logWriteStream.ts, packages/device-node/src/device/test/logCaptureProviders.test.ts, docs/memory/device-node/*, docs/memory/cli/session-runner.md	LogWriteStreamRegistry records the first stream error, surfaces it after cleanup, protects logging failures, and adds coverage for asynchronous failures, first-error behavior, quiet finalization, and successful writes.
Change scope and verification record fab/changes/260730-eyvt-ci-cost-guards-carried-defects/intake.md, fab/changes/260730-eyvt-ci-cost-guards-carried-defects/plan.md	The intake and plan define the workflow and stream changes, non-goals, acceptance scenarios, verification sequence, and design assumptions.
Change execution state fab/changes/260730-eyvt-ci-cost-guards-carried-defects/.history.jsonl, fab/changes/260730-eyvt-ci-cost-guards-carried-defects/.status.yaml	Pipeline history and status metadata record stage transitions, review outcomes, confidence values, impact metrics, and completion metadata.

Estimated code review effort: 4 (Complex) | ~45 minutes

Sequence Diagram(s)

sequenceDiagram
  participant GitHubEvents
  participant DriversWorkflow
  participant AndroidJob
  participant IOSJob
  GitHubEvents->>DriversWorkflow: Trigger on configured driver paths
  DriversWorkflow->>AndroidJob: Run with 15-minute timeout
  DriversWorkflow->>IOSJob: Run with 25-minute timeout

Loading

sequenceDiagram
  participant TestOrCaller
  participant LogWriteStreamRegistry
  participant WriteStream
  TestOrCaller->>LogWriteStreamRegistry: open(outputFilePath)
  LogWriteStreamRegistry->>WriteStream: Attach error listener
  WriteStream-->>LogWriteStreamRegistry: Record first error
  TestOrCaller->>LogWriteStreamRegistry: finalize()
  LogWriteStreamRegistry-->>TestOrCaller: Reject after cleanup

Loading

Possibly related PRs

• droid-ash/finalrun-agent#167: Modifies the same driver workflow and device log streaming implementation.

Suggested labels: bug

Suggested reviewers: arnoldlaishram

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
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 clearly summarizes the PR's two main themes: CI cost controls and fixes for carried-forward defects.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch 260730-eyvt-ci-cost-guards-carried-defects

---

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

[coderabbitai]

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 `@fab/changes/260730-eyvt-ci-cost-guards-carried-defects/plan.md`:
- Around line 142-157: Expand the registry regression-test scope from four to
six cases, including the two supplemental first-error cases, and keep all cases
in the existing logCaptureProviders.test.ts suite. Update
fab/changes/260730-eyvt-ci-cost-guards-carried-defects/plan.md lines 142-157 to
enumerate all six, line 224 to make the NICE-TO-HAVE status consistent with
required first-error coverage, and lines 245-254 to reflect the complete scope
in A-006/A-010. Update
fab/changes/260730-eyvt-ci-cost-guards-carried-defects/intake.md lines 336-346
to list all six cases and lines 431-434 to change the impact summary from four
cases to six.

🪄 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: f13c4ba0-aaf8-4327-b4f3-9c55c7378794

📥 Commits

Reviewing files that changed from the base of the PR and between 0ad8580 and 364d2a5.

📒 Files selected for processing (14)

• .github/workflows/drivers.yml
• docs/memory/ci/index.md
• docs/memory/ci/pr-quality-gate.md
• docs/memory/cli/session-runner.md
• docs/memory/device-node/index.md
• docs/memory/device-node/log-capture.md
• docs/memory/drivers/grpc-contract.md
• docs/memory/index.md
• fab/changes/260730-eyvt-ci-cost-guards-carried-defects/.history.jsonl
• fab/changes/260730-eyvt-ci-cost-guards-carried-defects/.status.yaml
• fab/changes/260730-eyvt-ci-cost-guards-carried-defects/intake.md
• fab/changes/260730-eyvt-ci-cost-guards-carried-defects/plan.md
• packages/device-node/src/device/logWriteStream.ts
• packages/device-node/src/device/test/logCaptureProviders.test.ts