Skip to content

fix(orchestration): word the worker brief for a chat worker - #26036

Merged
brennanb2025 merged 5 commits into
mainfrom
brennanb2025/chat-worker-brief-wording
Oct 7, 2026
Merged

brennanb2025 merged 5 commits into
mainfrom
brennanb2025/chat-worker-brief-wording

Conversation

@brennanb2025

@brennanb2025 brennanb2025 commented Oct 7, 2026 •

Copy link
Copy Markdown
Contributor
Files Added Deleted Net
Test 8 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​192 $\color{#cf222e}{\Huge{\mathbf{−}}}$​15 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​177
Prod 2 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​38 $\color{#cf222e}{\Huge{\mathbf{−}}}$​20 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​18

ELI5

When a coordinator agent hands a task to a worker agent, Orca sends the worker a set of instructions (the "worker brief"). That brief was written for a worker running in a terminal. A worker can now also be a chat: either a chat Orca starts itself for the task, or a chat someone already had open that is used as a worker (#22972). Those chats were told things like "the coordinator cannot see this terminal" and "Do not exit the shell. Your terminal stays available…", which is wrong for a chat. This PR words those lines for a chat. Terminal workers get exactly the same text as before.

What Changed

Before: a chat worker's brief said:

  • "The coordinator cannot see this terminal … a question or result left only in this terminal never gets to it."
  • "…the task would stall until someone happened to look at this terminal."
  • "Do not exit the shell. Your terminal stays available, and if the coordinator has more for you it will re-engage this terminal with a fresh preamble + TASK block, which arrives as new input."

After: a chat worker's brief says "this chat" in those places, and the last paragraph becomes:
"If the coordinator has more for you, it will send this chat a fresh preamble + TASK block, which arrives as a new message."
The brief also shows up as the task message in the chat, so a person reading that chat now sees this wording too.

Terminal workers' brief is unchanged, byte for byte. The existing snapshot test for it is untouched.

Mechanism: the brief builder (src/main/runtime/orchestration/preamble.ts) already checks whether the worker is named by an Orca session ID (orca_session_id:…) to choose how to introduce the worker. Every caller names a worker that way only when it is a chat (one Orca started, or one adopted as a worker); a terminal is always named by its terminal handle. This PR moves that check into one function, isSessionHandle, and uses it for both the name and the new wording. Nothing else changes: no new setting or flag, no callers touched, and the CLI commands in the brief (including the --terminal flag name) are the same.

Why

The worker's handle already says what kind of worker it is, and the builder already relies on it. Reading the same fact again means the name and the wording can't disagree. The other option was to add a "chat" value to the builder's workerKind option and have every caller set it. That adds a second flag that every caller would have to keep in step with the handle, and nothing sets workerKind today.

A chat that is passed workerKind: 'bare-shell' still gets the chat wording, because a chat has no shell to exit.

Linked Issue

Follow-up to #22972, which made a chat able to be a worker. That PR left this wording out of scope.

Visual Proof

chat-brief-before-after.png

This image shows the brief a chat worker receives, main (red) vs this PR (green). It is the brief builder's output, not a screenshot of a live chat. Unchanged lines are left out. The terminal worker brief is byte-identical on main and this PR (same sha256).

There is no layout, label or notice change; only the words of the brief differ. A live run was replaced by tests that read the text each real send path actually hands over (see Testing).

Testing

  • preamble.test.ts:

    • a new snapshot of the chat brief;
    • an exact comparison: the chat brief equals the terminal brief once the identity line, the worker's name, the three "this chat" words and the last paragraph are swapped back;
    • checks that a chat brief contains no "this terminal", "exit the shell" or "Your terminal", even with workerKind: 'bare-shell'.
      The existing terminal snapshot is unchanged; the snapshot file only gained lines.
  • Tests that read the text each real send path hands over:

    • A chat someone already had, used as a worker through worker-start --terminal orca_session_id:…: structured-chat-worker-start.test.ts.
    • The same kind of chat reached through orchestration dispatch --to orca_session_id:… --inject: structured-chat-dispatch-worker.test.ts.
    • A chat Orca starts as a worker through the workerStart handler: orchestration-worker-start-mode-selection.test.ts.
    • A terminal worker through workerStart and through dispatch --inject: still gets "cannot see this terminal" and "Do not exit the shell. Your terminal stays available".
  • Ablation: with the chat check forced off, every chat test failed (7) and both terminal tests passed. With the check restored, 100/100 pass.

  • orchestration-worker-mode-opacity.test.ts checks that a worker is taught the same thing whichever way it runs. It now treats the chat/terminal words and the last paragraph as expected differences, and checks that a chat gets the chat paragraph and a terminal keeps main's.

  • 356 tests passed across the 24 test files that build or check the brief. oxlint and the node typecheck are clean after merging current main.

  • I manually tested these changes locally

  • Automated tests added/updated, or explained why not below

Review

  • Plan reviewed by one reviewer before coding.
  • Round 1, two reviewers:
    • An adversarial reviewer rendered the brief on main and on this PR across 3,645 combinations of settings. Every terminal brief was byte-identical, and every path that reaches a chat got the chat wording.
    • An architecture reviewer sketched its own fix from the issue alone and arrived at the same design: the worker's handle is the signal, with no new flag, and the PR should not be split.
    • No P0–P3 issues. Two small nits were fixed: a code comment, and tests now import the two paragraphs instead of copying them.
  • Readiness checklist (remote, crashes, security, performance, correctness, compatibility, cross-platform, organization): pass. The only code that reads the brief's text keys on its first line, "Your task ID is:" and "=== TASK ===", none of which changed. Nothing stores a copy of the brief.

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

  • Not changed, on purpose:
    • When an existing chat is used as a worker, the worker-start receipt's fixed fields still read "terminal" (mode, reason: reused_terminal, effect kind terminal). These are machine values clients read, not wording. The human-readable line in that receipt already names the chat.
    • The nesting-limit refusal says "Do the work in this terminal instead of dispatching a sub-worker." That error is built without knowing what kind of worker asked, and rewording it would change terminal workers' text. Left for a follow-up.
  • SSH / remote: the brief is built by the host that delivers it to the worker. Clients only look at the brief's first line, which is unchanged. No wire change.
  • Cross-platform: text only.

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)
  • pnpm lint, pnpm typecheck, pnpm test, and pnpm build pass (or CI will cover; local preferred)

X / Twitter: @BrennanKB5

@brennanb2025

Copy link
Copy Markdown
Contributor Author

Status: ready for review

The problem. The brief Orca sends a worker agent was written for a terminal. A chat used as a worker, whether Orca started it for the task or a person already had it open (#22972), was told "the coordinator cannot see this terminal" and "Do not exit the shell. Your terminal stays available… re-engage this terminal". That is wrong for a chat, and because the brief also shows in the chat as the task message, a person reading the chat saw it too.

The user-facing change. A chat worker's brief now says "this chat" in those places, and its last paragraph says: "If the coordinator has more for you, it will send this chat a fresh preamble + TASK block, which arrives as a new message." Terminal workers get exactly the same text as before. The choice is made from the worker's own name: a chat is named by its Orca session ID. The brief builder already used that to introduce the worker, so there is no new flag.

Review.

  • Plan review: done.
  • Round 1:
    • An adversarial reviewer rendered main and this PR across 3,645 combinations of settings. Every terminal brief was byte-identical, and every path that reaches a chat got the chat wording.
    • An architecture reviewer designed a fix independently from the issue and arrived at the same design, with no split.
    • No P0–P3 issues. Two nits were fixed.
  • Readiness checklist: pass, with no P0/P1/P2.
  • One issue the reviews missed: CI caught orchestration-worker-mode-opacity.test.ts, which required a chat brief and a terminal brief to be identical. It was fixed by treating the chat/terminal words as an expected difference and checking each brief keeps its own paragraph.

Fixed:

  • The chat wording itself.
  • Tests that read the text each real send path hands over: a chat used as a worker through worker-start and through dispatch --inject, a chat Orca starts as a worker, and a terminal worker.
  • With the chat check forced off, every chat test fails and the terminal tests pass.

Deferred. Not changed here, because rewording them would change terminal workers' text or a machine-readable field:

  • When an existing chat is used as a worker, the worker-start receipt's fixed fields still read "terminal" (mode, reason: reused_terminal, effect kind). These are values clients read; the readable line already names the chat.
  • The nesting-limit refusal still says "Do the work in this terminal…" to any worker.
  • Two terminal-only CLI/error messages ("bound to this terminal", "Rebind this terminal").

CI.

  • Green on b2ea271 after merging current main.
  • One test shard was cancelled because the CI runner was shut down mid-run. That was infrastructure, not a test failure, and the re-run passed.
  • An earlier lint failure came from main's own breakage, which main fixed separately.

Verified:

  • Terminal text is byte-identical to main, by snapshot and by rendering every combination of settings.
  • Chat wording reaches both kinds of chat worker through the real send code, shown by tests and by forcing the check off.
  • 356 tests across the 24 test files that build or check the brief pass. Node typecheck and lint are clean.

Not verified:

  • No live run in the app. The visual proof is the brief builder's rendered output, labelled as such. Live QA was replaced by the send-path tests.
  • How an agent behaves with the new wording.

@brennanb2025
brennanb2025 marked this pull request as ready for review October 7, 2026 05:32
@coderabbitai

coderabbitai Bot commented Oct 7, 2026

Copy link
Copy Markdown
Contributor

Review in 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: b1e01b3f-c52d-4b0c-8b06-bc0f46ad1f50
📥 Commits

Reviewing files that changed from the base of the PR and between 7c9c3d4 and b2ea271.

⛔ Files ignored due to path filters (1)
  • src/main/runtime/orchestration/__snapshots__/preamble.test.ts.snap is excluded by !**/*.snap
📒 Files selected for processing (9)
  • src/main/runtime/orchestration/agent-facing-parity.test.ts
  • src/main/runtime/orchestration/preamble.test.ts
  • src/main/runtime/orchestration/preamble.ts
  • src/main/runtime/rpc/methods/orchestration-worker-mode-opacity.test.ts
  • src/main/runtime/rpc/methods/orchestration-worker-start-mode-selection.test.ts
  • src/main/runtime/rpc/methods/orchestration/runs/tasks-dispatch.test.ts
  • src/main/runtime/rpc/methods/orchestration/worker/deliver-worker-dispatch-preamble.ts
  • src/main/runtime/structured-chat-dispatch-worker.test.ts
  • src/main/runtime/structured-chat-worker-start.test.ts

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


📝 Walkthrough

Walkthrough

Dispatch preambles now distinguish chat workers from terminal workers by their session handles. The generated instructions use mode-specific identity, visibility warnings, and post-completion guidance. Tests check the resulting preambles and verify that structured and terminal worker modes use their corresponding prompt senders.

Priority: ⬇️ Low

Merge Risk: ⚪ Minimal · up to b2ea2

The revised worker briefs are ready to merge after normal checks; no concrete issue remains identified.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 66.67% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 9 functions across 9 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: updating the worker brief wording for chat workers.
Description check ✅ Passed The description covers the required sections with a clear summary, rationale, linked issue, visual proof, testing details, review notes, and completed checklist. It also explains why live testing was …
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 💡 2
📝 Generate docstrings 💡
  • Commit to this branch
  • Create a new PR
🛠️ Fix failing CI checks 💡
  • Commit to this branch
  • Create a new PR
  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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.

@brennanb2025
brennanb2025 merged commit 6bc5291 into main Oct 7, 2026
77 of 79 checks passed
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