Skip to content

fix(orchestration): honor structured worker transport admission - #20866

Closed
brennanb2025 wants to merge 3 commits into
mainfrom
brennanb2025/fix-structured-worker-admission
Closed

brennanb2025 wants to merge 3 commits into
mainfrom
brennanb2025/fix-structured-worker-admission

Conversation

@brennanb2025

@brennanb2025 brennanb2025 commented Sep 15, 2026 •

Copy link
Copy Markdown
Contributor
Files Added Deleted Net
Test 7 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​125 $\color{#cf222e}{\Huge{\mathbf{−}}}$​30 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​95
Prod 7 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​44 $\color{#cf222e}{\Huge{\mathbf{−}}}$​17 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​27

ELI5

Starting a new native Codex or Claude worker could immediately report an unknown outcome and tear down its session even though its task had been sent. Worker-start now succeeds when the structured host admits the preamble.

What Changed

  • Treat pending and accepted preamble results as successful admission; preserve distinct rejected and genuinely unknown outcomes.
  • Report turnStart: unsupported for this structured path, which has no correlated turn-start observer, instead of claiming an observed turn.
  • Keep mailbox pointers pending under the same operation ID until provider acceptance, without mislabeling pending admission as unknown.
  • Add both-provider integration and lifecycle regressions covering single delivery, readiness, rejection, uncertainty, hold release and tab cleanup.

Why

The host changed to return after transport admission, before a provider echo establishes message identity. Worker-start retained an accepted-only assumption and converted the normal pending result into operation_unknown. The fix aligns the consumer with host-owned admission; it introduces no additional delivery, polling or state store. Readiness confirms admission, while consuming durable mail still requires provider acceptance.

Linked Issue

User-reported native structured worker-start regression.

Visual Proof

N/A — runtime admission and CLI receipt correction; no rendered UI changes. Automated regressions reproduce both providers before an echo and verify the resulting behavior.

Testing

  • Four new regressions failed against unchanged origin/main production code.
  • 262 tests passed across 18 targeted suites, including both production adapters with fake provider transports, worker lifecycle/mode behavior, mailbox delivery, send idempotency and legacy RPC compatibility.
  • pnpm tc:node passed.
  • pnpm run check:code-quality:changed passed with zero findings.
  • Changed files formatted; git diff --check passed.
  • Tests ran on macOS with ORCA_BACKGROUND_LAUNCH=1. Live authenticated providers, Electron UI, Windows/Linux and SSH were not exercised; full build and broader CI checks remain for CI.

Review

Delivery remains a single host send. Pending admission preserves the worker hold; rejected and unknown results retain existing error classification and cleanup. No wire schema, provider protocol, SSH/WSL placement or folder-workspace logic changed.

Agent skill upstream boundary

  • Not applicable; no skill implementation changed.

Checklist

  • Focused runtime correction with regression tests.
  • Root cause and before/after behavior explained.
  • Self-reviewed for correctness, lifecycle ownership and performance.
  • Cross-platform, SSH/remote and mixed-version impact considered.

@coderabbitai

coderabbitai Bot commented Sep 15, 2026 •

Copy link
Copy Markdown
Contributor

Review Change StackReview Change Stack

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: 554db5e8-09bc-4646-bfc2-f4453a9871fd

📥 Commits

Reviewing files that changed from the base of the PR and between c8a7197091915646b0f427eabab9a871a6b61bea and c4f55d9.

📒 Files selected for processing (1)
  • src/shared/agent-status-legacy-adapter.ts

Included review availability: Your plan provides up to 10 included reviews per hour; 0 remain after this review.


📝 Walkthrough

Walkthrough

The change adds pending as a distinct structured dispatch state. Mail remains retained and reuses its operation until provider acceptance. Structured worker preambles now succeed for pending and accepted submissions. Worker start tests cover pending, accepted, rejected, and unknown outcomes for Codex and Claude. New integration coverage verifies fresh-session admission without resending the preamble.

Priority: ➖ Normal

Merge Risk: ⚪ Minimal · up to c4f55

The admission changes preserve pending delivery and readiness behavior as intended, with no remaining merge-blocking risk identified.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 36.36% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 11 functions across 14 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely describes the main change: honoring structured worker transport admission during orchestration.
Description check ✅ Passed The description is mostly complete and covers the change, rationale, testing, visual proof, review notes, and checklist. The Linked Issue section does not include an issue reference or a Fixes #... …
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.
  • Fix all pre-merge checks with AI
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Create stacked PR
  • Commit on current branch

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

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

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ No new issues found.

Reviewed changes — aligns the orchestration consumer of a structured session send with the host's transport-admission contract.

  • Preamble admission — sendStructuredWorkerPreamble now accepts pending as admitted, preserving rejected → dispatch_preamble_undelivered and everything else → operation_unknown.
  • Pointer-lane state — StructuredDispatchState gains pending with a distinct dispatch-pending retain reason; structuredDispatchDelivered still consumes mail only on accepted.
  • Pointer host mapping — pending passes through instead of collapsing to unknown, which is behavior-preserving for retain/park and only sharpens the reason label.
  • Turn-start honesty — structured worker starts report turnStart: 'unsupported' rather than a false 'observed', since this path has no correlated turn-start observer.
  • Regressions — a parameterized local-worker-start test over pending/accepted/rejected/unknown (codex + claude), both-provider integration tests asserting one send on pending admission, and a pointer-lane test pinning operation-ID replay until acceptance.

The change is grounded in the adapter contract: both providers return { state: 'admitted' } on the normal path and the host records that as a pending submission (structured-agent-session-adapter.ts:100-112), so the prior accepted-only check turned every fresh native worker start into outcome_unknown. The removed sendStructuredWorkerPreamble mock means the new failure test exercises the real function, so it fails against pre-fix code. No unresolved concerns.

Pullfrog  | View workflow run | Using DeepSeek Flash (free via Pullfrog for OSS) | 𝕏

@brennanb2025

Copy link
Copy Markdown
Contributor Author

Electron validation (Grok)

Live orchestration.workerStart against this PR worktree created a native structured Codex worker. The admitted pending preamble settled ready with turnStart: unsupported (mode: structured) instead of start_unknown / outcome_unknown.

Visible Orca: Codex Chat tab stayed up, showed the dispatch preamble, then ADMISSION_OK. Backing receipt: dispatch ctx_3a7de81e9098, worker structworker_f2abbd49-4b53-4fef-9c0b-a5146c3aebfa, state: ready, stage: input_accepted, dispatch_input: accepted.

After-only screenshot: reproducing the pre-fix start_unknown tear-down would require launching unmodified main, which this validation did not do.

after-structured-codex-worker.png

@brennanb2025
brennanb2025 force-pushed the brennanb2025/fix-structured-worker-admission branch from c8a7197 to c4f55d9 Compare September 15, 2026 18:11

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caution

Some comments are outside the diff and can’t be posted inline due to GitHub limitations.

⚠️ Outside diff range comments (1)

🟠 Major · Do not mark the structured worker ready for a pending preamble. · src/main/runtime/rpc/methods/orchestration-structured-worker-session.ts:248-249

248-249: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift

Do not mark the structured worker ready for a pending preamble. sendStructuredWorkerPreamble returns successfully for pending, so readiness settlement records the dispatch as accepted and marks the worker ready. Later journal reconciliation can reject that submission, but it only updates the journal. It does not tear down the ready worker or call releaseStructuredWorkerSession, so the ready receipt and structured-session hold can remain active without an admitted task. Defer readiness until acceptance, or explicitly tear down the worker and release the hold when reconciliation rejects the pending submission.


ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Advanced

Run ID: 9760bda1-7beb-413d-b5ef-1685f41036aa

📥 Commits

Reviewing files that changed from the base of the PR and between 11733f2 and c8a7197091915646b0f427eabab9a871a6b61bea.

📒 Files selected for processing (1)
  • src/shared/agent-status-legacy-adapter.ts

Included review availability: Your plan provides up to 10 included reviews per hour; 0 remain after this review.

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Important

The incremental commit adds a typescript/consistent-type-assertions violation that fails CI's changed-code quality gate.

Reviewed changes — re-review of the single commit since the prior pullfrog review (c8a7197); the main merge brought no other net change to the PR.

  • Reflective read removed — freezeRecursively now reads own properties via indexed access instead of Reflect.get, satisfying anti-slop/no-reflect-get; the read semantics are unchanged (own keys, same getter receiver).
  • Casting gate now fails — the replacement's as Record<PropertyKey, unknown> assertion is not suppressed, so the changed-code casting scan reports it on the added line.

Pullfrog  | Fix all ➔ | Fix 👍s ➔ | View workflow run | Using DeepSeek Flash (free via Pullfrog for OSS) | 𝕏

Comment thread src/shared/agent-status-legacy-adapter.ts Outdated

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ No new issues found.

Reviewed changes — delta since the prior pullfrog review (c8a7197 → c4f55d9).

  • Casting suppression corrected — the bare // SAFETY: comment on the legacy adapter's indexed read is now the repo's directive form (// oxlint-disable-next-line typescript/consistent-type-assertions -- SAFETY: ...), clearing the changed-code casting finding from the previous review.

Pullfrog  | View workflow run | Using DeepSeek Flash (free via Pullfrog for OSS) | 𝕏

@brennanb2025

Copy link
Copy Markdown
Contributor Author

Validation status

The structured native worker admission regression is fixed and the PR is ready for merge.

Review and readiness

  • Astra high review loop completed clean after one narrow in-scope correction. Report: /tmp/pr20866-review-loop-2.md
  • Final readiness checklist passed with no proven P0, P1, P2, or P3 findings. Report: /tmp/pr20866-readiness-final.md
  • Local validation: 83 focused tests, Node typecheck, changed-file quality, anti-slop, formatting, hooks, and diff checks passed.
  • GitHub PR Checks: all required jobs passed, including static analysis, typecheck, Linux/Windows packaging, eight Node 24 test shards, and verify.

Electron evidence

Grok ran live orchestration.workerStart in a background Electron build of this branch. The pending preamble settled ready with turnStart: unsupported, the native Codex session displayed ADMISSION_OK, and the session remained available. Screenshot and details: #20866 (comment)

Remaining gaps are limited to full-repository test/lint runs and live SSH/WSL network fault injection; the changed paths add no new remote transport behavior.

@brennanb2025

Copy link
Copy Markdown
Contributor Author

Closing this older worker-start patch. Current main keeps a worker whose task is still pending and reports whether its turn was actually observed; #22972 and #26724 replaced the old flow.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant