Repository navigation
Conversation
|
Important Review skippedWe couldn't safely recover the incremental review. No full review was started, and the last reviewed checkpoint was preserved. Retry later, or explicitly request a full review by commenting You can disable this status message by setting the Use the checkbox below for a quick retry:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Walkthrough
Priority: ➖ Normal Merge Risk | 🔵 Low · up to
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage | 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 3 functions across 5 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 Issue #24509requires Kimi follow-up prompts to submit with CR, not LF. When the readiness scanner confirms the composer,sendWorktreeStartupFollowupWhenReadysends one bracketed-paste frame, waits …Out of Scope Changes check ✅ Passed The agent field, readiness handling, and focused test updates support the fix for issue #24509. The changes remain within the linked issue's scope. No unrelated change is demonstrated.Title check ✅ Passed The title clearly identifies the main change: framing stdin-after-start follow-up prompts through the paste protocol. Description check ✅ Passed The description covers the required sections, explains the change and its reason, links issue #24509, documents testing and platform considerations, and marks visual proof as not applicable with a rea…
- Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Autopilot is currently an internal CodeRabbit preview.
Comment @coderabbitai help to get the list of available commands.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Advanced
Run ID: 4a52b569-f936-4773-add9-f6c00ea05359
📥 Commits
Reviewing files that changed from the base of the PR and between efbf651 and 2550e3ff0638d3792d3a6a99a7d91896a045c997.
📒 Files selected for processing (5)
src/main/runtime/orca-runtime-tests/worktree-setup-and-startup-part-04.spec.tssrc/main/runtime/runtime-worktree-agent-startup.tssrc/main/runtime/runtime-worktree-startup-readiness.test.tssrc/main/runtime/runtime-worktree-startup-readiness.tssrc/main/runtime/terminal-run-facts-input.test.ts
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review.
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
Important
The runtime funnel fix is correct, but one sibling stdin-after-start follow-up path was not updated: the desktop renderer's sendFollowupPromptWhenAgentReady still writes the raw prompt + \r. That path is live for desktop workspace creation, so the same failure the PR fixes on the runtime path can still occur there. Separately, the new readiness gate cannot resolve on Windows ConPTY panes, so it adds a fixed ~8s delay before the existing process-name fallback. Details below.
Reviewed changes
- Follow-up paste framing —
sendWorktreeStartupFollowupWhenReadynow gates onwaitForWorktreeStartupDraft, writes the prompt as a single bracketed-paste frame, waits the agent's submit-settle delay, then writesAGENT_PROMPT_SUBMITas its own write. WorktreeStartupFollowup.agent— new required field, populated at the single construction site inbuildWorktreeStartupForAgent. It does not cross a wire schema (RuntimeManagedWorktreeCreateArgscarries onlystartupDraftPaste, which already hadagent), so no protocol-version impact.- Tests — new unit regression pinning one paste frame plus a lone CR, and magnetic assertion updates in the integration specs (2 → 3 writes).
⚠️ The desktop renderer follow-up path still raw-writes the prompt
sendWorktreeStartupFollowupWhenReady is only the runtime half of the stdin-after-start delivery. The desktop renderer's sendFollowupPromptWhenAgentReady still writes ${prompt}\r in one raw write, gated only on the foreground process — the exact shape this PR removes. It is reachable: for a stdin-after-start agent startupPlan.followupPrompt is set, which leaves backendStartup undefined, so the runtime never spawns a startup terminal and ensureAgentStartupInTerminal → deliverAgentStartupToTerminal takes the follow-up branch. The same LF-vs-CR and multi-line keystroking failure can therefore still bite desktop workspace creation even though the automation/worker path is fixed.
Technical details
# Desktop renderer follow-up path is unfixed
## Affected sites
- `src/renderer/src/lib/agent-followup-delivery.ts:25` — writes `${prompt}\r` raw: no bracketed-paste frame, no composer-ready gate
- `src/renderer/src/lib/new-workspace.ts:289` — the `startup.followupPrompt` branch that calls it
- `src/renderer/src/hooks/composer-state/full-submit-preparation.ts:215` — leaves `backendStartup` undefined whenever `followupPrompt` is set, forcing the renderer path
## Required outcome
- Decide whether desktop workspace creation with a stdin-after-start agent is in scope for #24509. If it is, route this path through the same bracketed-paste + lone-CR protocol (ideally via one shared helper) so the two funnels cannot drift again.
## Open questions for the human
- Is this path intentionally out of scope (automation/worker dispatch only), or tracked in a separate issue?deepseek-v4.1-flash (free via Pullfrog for OSS) ｜ 𝕏
| void waitForWorktreeStartupFollowup(host, handle, followup.expectedProcess) | ||
| .then((ptyId) => { | ||
| if (!ptyId) { | ||
| void waitForWorktreeStartupDraft(host, handle, followup.agent) |
There was a problem hiding this comment.
On a Windows ConPTY pane the composer-ready signal can never fire — ConPTY swallows the agent's \x1b[?2004h, which the repo's own tests assert (draft-paste-ready-scanner.test.ts:275, agent-paste-draft-readiness-budget.test.ts:99). Because the process-name fallback is sequenced behind this call, every stdin-after-start follow-up on Windows now blocks for the full resolveDraftPasteReadyTimeoutMs (8s) before it is delivered, where the old code delivered as soon as the agent was foreground. Racing the two gates (or skipping the readiness wait when the PTY write host is win32) keeps the fallback without the added latency.
There was a problem hiding this comment.
Verified the mechanism but the proposed race would reintroduce the root cause: the process-name gate fires almost immediately after exec — that early fire is exactly the #26387 measurement (foreground name matching at ~0.03s while the REPL is still forming), so min(readiness, process-name) resolves to process-name in practice and the prompt lands before the composer can submit it. The readiness timeout is the cost of the gate being output-confirmed; the fallback still delivers (bare write) on agents whose ready signal never fires.
The latency only materializes on hosts where the signal truly never arrives. If ConPTY does swallow DECSET 2004h end-to-end, Windows pays the full timeout before fallback — if a maintainer can confirm that, a shorter win32-aware timeout (or a platform-conditional gate skip) looks like the safer lever than racing, and I'm happy to implement it on that evidence. I could not verify the swallow claim from the repo itself: the cited tests (draft-paste-ready-scanner.test.ts:275, agent-paste-draft-readiness-budget.test.ts:99) assert Codex composer-glyph sequencing and readiness budgets, not ConPTY DECSET handling.
|
Addressed in 607d4ae071: the scanner-confirmed path keeps the paste frame + lone CR; the process-name fallback restores the bare |
There was a problem hiding this comment.
ℹ️ No new issues in this delta. The fallback path was correctly restored to a bare write, and the tests now pin both branches. The Windows ConPTY readiness latency raised in the previous review is unaffected by this commit and remains open.
Reviewed changes
- Fallback delivery reverted to a bare write — when the composer-ready scanner times out,
sendWorktreeStartupFollowupWhenReadynow writes${prompt}${AGENT_PROMPT_SUBMIT}in a single raw write instead of the bracketed-paste frame, on the reasoning that bracketed paste may be disabled on that path, so theESC[200~/ESC[201~delimiters would type in as literal input. - Tests updated — the fallback unit case pins the single
prompt + CRwrite, while the scanner-confirmed case keeps pinning one paste frame plus a lone CR.
The scanner-confirmed paste path and the WorktreeStartupFollowup.agent field are unchanged from the previous commit.
deepseek-v4.1-flash (free via Pullfrog for OSS) ｜ 𝕏
607d4ae to
eed718a
Compare
|
Rebased onto current main (990b0ed) to clear the merge conflict in |
|
Confirmed the finding against the current head: for a stdin-after-start agent the composer path leaves Scope call: this PR fixes the runtime dispatch funnel that #24509 is about (automation/worker). Covering the renderer path means giving it the same output-confirmed readiness seam the runtime funnel uses (the runtime side subscribes to PTY output for the ready scanner; the renderer currently has no equivalent for this path) plus the same bare-write fallback semantics — a behavior change to a live desktop delivery path that I'd rather not fold into an already heavily-reviewed PR. Happy to open a follow-up PR routing |
…aste protocol Automation and worker dispatch delivered `stdin-after-start` agents' follow-up prompts as one raw write (`prompt` + CR) gated only on the foreground process name. The write lands while the TUI is still switching its tty to raw mode, so the kernel line discipline (ICRNL) translates the CR to LF before the child reads it. Kimi-style TUIs bind LF to insert-newline, so the prompt stayed unsubmitted in the composer while the run was marked completed (stablyai#24509). Deliver the follow-up through the same protocol every other TUI dispatch already uses: wait for the composer-ready draft signal (falling back to the process-name gate when the signal never fires), write the prompt as a single bracketed-paste frame, wait out the submit settle delay, then write `AGENT_PROMPT_SUBMIT` ('\r') on its own. Multi-line prompts now arrive inside the paste frame instead of keystroking their embedded LFs. Fixes stablyai#24509 Generated-by: GLM-5.3-Flash (ZCode)
Coderabbit review: when the composer-ready signal times out, the process-name fallback can hit a TUI with bracketed paste disabled, where paste delimiters type in as literal input and the submit CR can still be line-discipline translated. The scanner-confirmed path keeps the paste frame + lone CR; the fallback restores the bare `prompt + CR` write this PR replaced. Generated-by: GLM-5.3-Flash (ZCode)
Rebase adaptation for upstream stablyai#26820: WorktreeStartupFollowup now carries the TUI agent (used to pick the composer-ready signal and the submit settle delay), so the headless automation's deliverTerminalPrompt passes it and the launch tests assert the full call shape. Generated-by: GLM-5.3-Flash (ZCode)
eed718a to
88f8abd
Compare
|
Rebased onto upstream main (now includes #26820, which extracted the awaitable
Verification on the rebased head |
This branch has not been deployed
1 participant
ELI5
When an Orca automation or worker hands a prompt to a terminal agent that expects the prompt typed into its composer (Kimi Code and similar TUIs), Orca typed the text and pressed Enter — but the "Enter" reached the agent as a newline keystroke instead. The text just sat in the input box and the run was reported as completed even though nothing happened. This PR delivers those prompts the same way every other agent-dispatch path in Orca already does, so Enter actually submits.
What Changed
sendWorktreeStartupFollowupWhenReady(the funnel every managed-worktree dispatch — local, SSH, folder, serve-mode — uses forstdin-after-startagents) previously wrote the follow-up prompt as one rawprompt + CRwrite, gated only on the foreground process name. That raw write can land while the TUI is still switching its tty into raw mode, so the kernel line discipline (ICRNL) translates the CR to LF before the child reads it. Kimi-style TUIs bind LF to insert-newline: the prompt stays in the composer with a trailing empty line and is never submitted (#24509).Now the follow-up uses the same delivery protocol as every other TUI dispatch path:
waitForWorktreeStartupDraftfor the follow-up's agent — the same gate draft pastes and fix(zcode): wait for composer before first worker dispatch #23374 already use), falling back to the existing process-name gate if the signal never fires;buildAgentPromptPasteBytes), so multi-line prompts arrive atomically instead of keystroking their embedded LFs;resolveAgentPromptSubmitDelayForAgent), then writeAGENT_PROMPT_SUBMIT('\r') as its own write.WorktreeStartupFollowupgains theagentfield this gate needs (single construction site updated). The user-visible before/after: an automation targeting Kimi Code now actually starts a session from its prompt instead of silently leaving it in the composer.Why
The byte that reaches the agent is the contract; where the raw write broke it was timing (pre-raw-mode line discipline), not a wrong literal — every dispatch site already writes
\r. Routing the one non-conforming path through the shared paste protocol fixes the root cause at the shared funnel rather than special-casing agents or callers, and brings multi-line prompt atomicity for free. The composer-ready gate is the same mechanism draft pastes already trust, so no new readiness concept is introduced.Linked Issue
Fixes #24509
Visual Proof
N/A — no Orca UI surface changes; the fix is byte-level behavior inside a third-party TUI session. The before/after at the protocol level is locked by the new test: one bracketed-paste frame
ESC[200~…ESC[201~followed by a lone\rwrite, and no write ending in\n.Testing
Automated: new regression test
runtime-worktree-startup-readiness.test.ts("frames the follow-up prompt as one bracketed paste and submits it with a lone CR") — fails on the old raw write (no paste frame, no standalone CR), passes after the fix. Updated the magnetic assertions inworktree-setup-and-startup-part-04.spec.tsandterminal-run-facts-input.test.tsto the new protocol. Local matrix:pnpm tcclean; oxlint clean on changed files; the key integration case ("sends follow-up prompts for CLI-created stdin-after-start startup agents") passes; fullpnpm test/pnpm buildnot run locally (CI covers them). Manual end-to-end against a real Kimi CLI was not run in this environment — the byte-level behavior is covered by the tests above.AI Disclosure
Implemented by GLM-5.3-Flash (ZCode agent) under direction of ggbdpq, who reviewed the diff and re-ran the key tests.
Review
Agent skill upstream boundary
docs/reference/agent-skill-sharing-upstream-boundary.mdand copies or mechanically translates no upstream skill-installer source, tests, fixtures, registry entries, path tables, comments, or documentation.Notes
Cross-platform: the settle delay is platform-aware (ConPTY ingest-rate model on win32, POSIX default otherwise); SSH/folder/serve-mode dispatch all funnel through the changed function and now get the identical protocol. No new dependencies, no persistence or protocol-version impact, backwards compatible for agents whose ready signal fires as before.
Checklist
N/Awith reasonpnpm lint,pnpm typecheck,pnpm test, andpnpm buildpass (or CI will cover; local preferred)