[brennanb2025]

	Files	Added	Deleted	Net
Test	3	$\color{#1a7f37}{\Huge{\mathbf{+}}}$​46	$\color{#cf222e}{\Huge{\mathbf{−}}}$​12	$\color{#1a7f37}{\Huge{\mathbf{+}}}$​34
Prod	1	$\color{#1a7f37}{\Huge{\mathbf{+}}}$​3	$\color{#cf222e}{\Huge{\mathbf{−}}}$​27	$\color{#cf222e}{\Huge{\mathbf{−}}}$​24

ELI5

The desktop app could not build after the Codex turn-boundary change: background-task handling still imported a file that the change removed. With this fix, the app builds again and a Codex child task stays in progress until Codex actually finishes its turn.

What Changed

Removed the stale import and the old path that treated a non-retried error event as the end of a child turn. The existing turn/completed event now settles a failed child turn. If its thread closes without completion, the result remains unverifiable. Tests cover both transitions in the task strip and persisted task record.

Why

#23682 deleted the imported verdict module while moving turn ownership to explicit turn boundaries. Restoring that module would revive the old assumption that an error always ends a turn, even though Codex can emit an error before its final completion event. This narrow change follows the new boundary and removes the build break without adding another source of lifecycle state.

Linked Issue

No separate issue; follow-up to the regression introduced by #23682.

Visual Proof

N/A — this repairs a compile failure and task-settlement behavior without changing UI layout or copy. The event transitions are covered by focused tests.

Testing

• I manually tested these changes locally
• Automated tests added/updated, or explained why not below

On macOS, tc-gate pnpm tc:node, 68 targeted tests across four files, pnpm run check:code-quality:changed, and ORCA_BACKGROUND_LAUNCH=1 pnpm build:electron-vite passed. Windows, Linux, and SSH were reviewed for impact but not run for this follow-up; the change adds no platform-specific path or process behavior. CI covers the remaining suites.

AI Disclosure

Review

Agent skill upstream boundary

• Not applicable, or this change follows docs/reference/agent-skill-sharing-upstream-boundary.md and copies or mechanically translates no upstream skill-installer source, tests, fixtures, registry entries, path tables, comments, or documentation.

Notes

No new process launch, network call, polling, or data persistence. The existing thread-close fallback remains in place for a child whose completion cannot be observed.

Checklist

• This PR is small and focused
• I explained what changed and why (ELI5, the user-facing before/after, the mechanism, and why over the alternatives)
• Before/after screenshots or videos attached for UI changes, or N/A with reason
• Self-reviewed for correctness, security, and performance
• Cross-platform, SSH/remote, and path/shortcut impact considered (or N/A)
• Scoped typecheck, targeted tests, changed-file lint, and Electron build pass; CI covers the full suite

[greptile-apps]

Confidence Score: 5/5

[Medium risk] Refactors how child task turns end in the background task system.

The PR appears safe to merge; the previous adapter-level test finding is addressed and no new actionable issue was identified.

Summary

The PR removes a stale import and changes child-task settlement to follow Codex turn completion rather than a non-retried error.

• Updates tests to check that an error leaves the child working until turn completion, while a thread close without completion leaves the outcome unverifiable.
• Updates the adapter-level fixture to follow the new turn boundary, addressing the previous review finding.

_{Reviews (2) · Last reviewed commit: "test(native-chat): wait for final Codex ..."}

[coderabbitai]

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Advanced

Run ID: fa1547a8-bb40-4ac0-80ff-2f097c639cf7

📥 Commits

Reviewing files that changed from the base of the PR and between 2d8e9ed and c4c2355.

📒 Files selected for processing (1)

• src/main/codex/codex-structured-child-work-producer.test.ts

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 3 remain after this review.

---

📝 Walkthrough

Walkthrough

The frame reader no longer uses provider verdicts to classify child-turn failures. It returns an unverifiable turn-ended frame when a non-primary thread closes. Tests cover non-retried errors that remain active until failed-turn completion, and child-thread closure before turn completion.

Priority: ⬇️ Low

Merge Risk: ⚪ Minimal · up to c4c23

A child remains active until completion or thread closure, while session shutdown clears its task state. No actionable merge risk remains.

Security Architecture Review

Security architecture risk: 🔵 Low · up to c4c23

The change keeps child work in progress until a turn actually completes and treats an incomplete closing thread as unverified. No new public access or control bypass was identified. Confidence is limited by incomplete visibility into restart recovery.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

• inferred — The identified effect is on Codex child-task status and its recorded outcome. The changed test callback does not add attacker-controlled production reachability; no wider security exposure was established.

Trust Boundaries and Controls

• inferred — Scoped consumer evidence indicates that child evidence still requires its structured-session parent and that prior invocations are fenced from changing the current run. The PR does not show a change to those authority checks.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 4 files.	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.
Title check	✅ Passed	The title clearly identifies the main change: restoring the build after the Codex turn-boundary update. It is concise and specific.
Description check	✅ Passed	The description is complete and focused. It explains the behavior change, motivation, testing, visual impact, platform considerations, and checklist status. It references #23682 as the related issue, …

• Fix all pre-merge checks with AI

✨ Finishing Touches 💡 1

📝 Generate docstrings 💡

• Commit to this branch
• Create a new PR

---

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.}

[pullfrog]

Important

The production change is sound and matches the captured transcript evidence, but the PR leaves a sibling test still asserting the removed behavior, so the suite goes red.

Reviewed changes

• Drop the stale verdict import — codex-background-task-frames.ts no longer imports readCodexProviderVerdict (the module deleted by #23682), and the CodexChildTurnEnding alias is gone. This restores the node build.
• Child turns now settle on turn/completed only — a non-retried error no longer produces a turn-ended frame; a child's failed turn settles from turn/completed's failed status. thread/closed still settles a child as unverifiable.
• Updated the tracker and child-work evidence tests — both now assert a child stays working through a non-retried error and settles on completion, and that thread/closed yields unknown.

The direction is correct: the real captures in src/main/codex/__fixtures__/codex-app-server-turn-endings.jsonl show every fatal error (willRetry: false) followed immediately by a turn/completed with status: "failed", and codex-structured-journal-turn-endings-replay.test.ts already pins that contract at the journal layer. This PR makes the child background tracker consistent with it. pnpm tc:node passes.

⚠️ A sibling test still asserts the removed "an error ends the turn" behavior

src/main/codex/codex-structured-child-work-producer.test.ts:335-344 feeds child l1 a non-retried error with no turn/completed, then expects byDescription('lint') to be { membership: 'settled', outcome: 'failed' }. Under the new contract the child stays live, so that step fails (expected { … } to match object { membership: 'settled' … }), and the final record assertion at lines 402-406 (lint → settled/failed) rides on the same step. The PR updated two of the three tests that encoded the old behavior; this third one was missed, so CI will fail. Reproduce with pnpm test src/main/codex/codex-structured-child-work-producer.test.ts (1 of 7 fails).

Technical details

# Stale child-work producer test

## Affected sites
- `src/main/codex/codex-structured-child-work-producer.test.ts:335-344` — the step sends only `{ method: 'error', params: { threadId: LINTER, turnId: 'l1', willRetry: false, error: { message: 'boom' } } }` and asserts `membership: 'settled', outcome: 'failed'`. `readCodexBackgroundTaskFrame` now returns `null` for this frame, so the child remains `live`.
- `src/main/codex/codex-structured-child-work-producer.test.ts:402-406` — the final records list expects `lint` at `settled`/`failed`, which also depends on the omitted completion.

## Required outcome
- The scenario must model what Codex actually emits: keep the `live`/`working` assertion after the `error`, then send `turn/completed` for `l1` with `status: 'failed'` and assert `settled`/`failed` there. The comment at line 335 ("…and no turn/completed") and the final records list need to line up with that.
- Prefer mirroring the shape already used in the updated `codex-background-task-tracker.test.ts` and `codex-child-work-evidence.test.ts` tests rather than inventing a new one.

## Open questions for the human
- The removed code assumed a non-retried `error` can end a **child** turn with no `turn/completed`. Do `spawn_agent` child threads share the primary thread's guarantee (per the fixture) that a fatal error is always followed by `turn/completed`? The fixture only covers primary threads. If a child can be left without a completion, its row stays `working` until `thread/closed` or session end — the PR argues the thread-close fallback and session clear cover that, but the child case is not exercised by a test either way.

  ^{｜ Fix it ➔ ｜ View workflow run ｜ Using DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏}

[pullfrog]

✅ No new issues found.

Reviewed changes

• Added the missing turn/completed step to the structured child-work producer test (codex-structured-child-work-producer.test.ts:345-350), so the lint child now settles from its failed completion rather than the non-retried error frame, with a live/working assertion after the error. This resolves the prior review's must-address finding: the sibling test now encodes the new contract, and the final lint → settled/failed record assertion lines up.
• Confirmed locally: all three touched test files pass (62 tests), including codex-structured-child-work-producer.test.ts and pnpm tc:node.

The production change (child turns settle on turn/completed only) was vetted in the prior review against the captured transcript fixture; this delta is test-only and brings the last test in line with it.

The open question from the prior review still stands and is not re-raised: whether spawn_agent child threads share the primary thread's guarantee that a fatal error is always followed by turn/completed. The fixture only covers primary threads, so the child case remains exercised only by the thread-close fallback.

  ^{｜ View workflow run ｜ Using DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏}

[brennanb2025]

Closing as superseded. #23801 removed the stale import and restored the build; #23808 then retained non-retried child errors as a terminal signal when no turn/completed arrives. Both are merged on main, and #23808 passed its CI checks. Rebasing this PR would undo that child-lifecycle fix.