Repository navigation
fix(terminal): verify agent prompt submission - #14962
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe runtime now tracks agent lifecycle activity per PTY generation and verifies prompt submissions within an abort-aware timeout. It serializes writes, rejects blocked or stale requests, handles cancellation, and reports successful submit counts. RPC forwarding preserves submission error codes and abort signals. Runtime, verification, and E2E tests cover lifecycle transitions, permissions, redraw stalls, concurrency, generation changes, swallowed Enter input, and structured error reporting. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/main/daemon/headless-emulator.test.ts (1)
56-63: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd a regression test for text after the cursor.
Both assertions leave the cursor at the end of the content. They do not prove that
getVisibleTextBeforeCursor()excludes text to the right of the cursor. Write a suffix, move the cursor left, and assert that the suffix is absent.Proposed test extension
await emulator.write('\r\nfooter') expect(emulator.getVisibleTextBeforeCursor()).toBe('header\nreview this now\nfooter') + await emulator.write('suffix') + await emulator.write('\x1b[6D') + expect(emulator.getVisibleTextBeforeCursor()).toBe('header\nreview this now\nfooter')
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 80a57668-f40b-4fa2-b4db-3d57fef0e155
📒 Files selected for processing (7)
src/main/daemon/headless-emulator.test.tssrc/main/daemon/headless-emulator.tssrc/main/runtime/agent-prompt-submission-runtime.test.tssrc/main/runtime/agent-prompt-submission-verification.test.tssrc/main/runtime/agent-prompt-submission-verification.tssrc/main/runtime/orca-runtime.test.tssrc/main/runtime/orca-runtime.ts
Included review availability: Your plan includes up to 10 reviews per rolling hour; 9 remain after this review.
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 97604720-1e82-498f-b46c-0e56b6c94b9b
📒 Files selected for processing (9)
src/main/daemon/headless-emulator.test.tssrc/main/runtime/agent-prompt-submission-runtime.test.tssrc/main/runtime/agent-prompt-submission-verification.test.tssrc/main/runtime/agent-prompt-submission-verification.tssrc/main/runtime/orca-runtime.tssrc/main/runtime/rpc/errors.test.tssrc/main/runtime/rpc/errors.tssrc/main/runtime/rpc/methods/terminal.tssrc/main/runtime/rpc/terminal-agent-prompt-send.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/daemon/headless-emulator.test.ts
Included review availability: Your plan includes up to 10 reviews per rolling hour; 8 remain after this review.
|
Post-review validation update:
The follow-up review also caught and fixed two race cases: a pre-paste remote/provider snapshot can no longer be reused as retry proof, and activity racing the final retry fence now requires fresh composer evidence instead of being accepted unconditionally. |
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/runtime/orca-runtime.ts (1)
17941-17967: 🎯 Functional Correctness | 🔴 Critical | 🏗️ Heavy liftImplement the guarded Enter retry.
verifyAgentPromptSubmissionchecks only generation and lifecycle status. It has no terminal snapshot, cursor reader, or PTY write callback.writeTerminalAgentPromptcalls it once and always returns1, so it cannot verify prompt placement or resend\r. Add the cursor-aware check and retry Enter once under the stated conditions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: c0e914a3-1f19-45cb-af19-f389044058bb
📒 Files selected for processing (6)
src/main/runtime/agent-prompt-submission-runtime.test.tssrc/main/runtime/agent-prompt-submission-verification.test.tssrc/main/runtime/agent-prompt-submission-verification.tssrc/main/runtime/orca-runtime.tstests/e2e/terminal-send-agent-prompt-submit.spec.tstests/tools/repro-terminal-send-submit.mjs
Included review availability: Your plan includes up to 10 reviews per rolling hour; 4 remain after this review.
| const deadline = Date.now() + AGENT_PROMPT_EFFECT_TIMEOUT_MS | ||
| while (Date.now() < deadline) { |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
file="src/main/runtime/agent-prompt-submission-verification.ts"
printf '%s\n' '--- target file ---'
cat -n "$file"
printf '%s\n' '--- related symbols and tests ---'
rg -n --glob '!node_modules' \
'verifyAgentPromptSubmission|AGENT_PROMPT_EFFECT_TIMEOUT_MS|performance\.now|Date\.now|useFakeTimers|toFake' \
src test tests package.json 2>/dev/null | head -n 300
printf '%s\n' '--- TypeScript/compiler and test configuration ---'
fd -i -t f '^(tsconfig.*|vitest.*|jest.*|package\.json)$' . -E node_modules -E dist -E build | sortRepository: stablyai/orca
Length of output: 33980
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- target references and nearby test files ---'
rg -n --glob '!node_modules' \
'agent-prompt-submission-verification|verifyAgentPromptSubmission|agent_prompt_stalled' \
. | head -n 200
printf '%s\n' '--- main/runtime and test configuration ---'
cat -n config/tsconfig.node.json
cat -n config/vitest.config.ts
cat -n package.json | sed -n '1,220p'
printf '%s\n' '--- runtime globals and fake-timer configuration ---'
rg -n --glob '!node_modules' \
'environment:|globals:|fakeTimers|toFake|doNotFake|performance' \
config tsconfig.json src/main src/shared | head -n 250Repository: stablyai/orca
Length of output: 50369
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- verification tests ---'
cat -n src/main/runtime/agent-prompt-submission-verification.test.ts
printf '%s\n' '--- Vitest version and lockfile entries ---'
rg -n '"vitest"|"`@sinonjs/fake-timers`"|vitest@' package.json pnpm-lock.yaml package-lock.json yarn.lock 2>/dev/null | head -n 80
printf '%s\n' '--- focused fake-timer examples ---'
rg -l --glob '*.test.*' 'vi\.useFakeTimers\(' src config tests | head -n 12 | while read -r f; do
printf '\n--- %s ---\n' "$f"
rg -n -A8 -B4 'vi\.useFakeTimers\(|vi\.advanceTimers|vi\.setSystemTime|performance\.now' "$f" | head -n 100
done
printf '%s\n' '--- Node runtime behavior ---'
node - <<'JS'
console.log(JSON.stringify({
node: process.version,
globalPerformance: typeof globalThis.performance,
performanceNow: typeof globalThis.performance?.now,
timerClock: Date.now(),
monotonicClock: globalThis.performance?.now()
}))
JSRepository: stablyai/orca
Length of output: 36069
🌐 Web query:
Vitest 4.1.5 vi.useFakeTimers performance.now advanceTimersByTime documentation
💡 Result:
In Vitest 4.1.5, performance.now() is not automatically mocked by vi.useFakeTimers() by default, even though some documentation may suggest otherwise [1][2][3]. To mock performance.now() and ensure it advances when using vi.advanceTimersByTime(), you must explicitly include 'performance' in the toFake option when calling vi.useFakeTimers() or configuring fakeTimers in your Vitest configuration file [1][2][4][3]. Usage Examples: In a test file: vi.useFakeTimers({ toFake: ['setTimeout', 'performance'] }); In vitest.config.ts: export default defineConfig({ test: { fakeTimers: { toFake: ['setTimeout', 'performance'], // Include other desired timers as needed }, }, }); When performance is correctly added to toFake, vi.advanceTimersByTime(ms) will cause performance.now() to advance by the specified number of milliseconds alongside other mocked timers [1][3]. If it is not included, performance.now() will continue to use the system's real-time clock and will not reflect the simulated time progression [1][2][3].
Citations:
- 1: vi.useFakeTimers() should also mock performance.now() vitest-dev/vitest#9351
- 2: Fake timers don't work for
performance.now()vitest-dev/vitest#4004 - 3: vi.useFakeTimers() doesn't fake performance by default despite docs claiming it does vitest-dev/vitest#9352
- 4: https://vitest.dev/config/faketimers
🏁 Script executed:
#!/bin/bash
set -eu
node - <<'JS'
function simulateDeadline(clock, adjustments) {
let now = 0
const deadline = clock(now) + 5_000
let elapsed = 0
let polls = 0
while (clock(elapsed) < deadline && polls < 1_000) {
elapsed += 50
polls += 1
if (adjustments[elapsed] !== undefined) now = adjustments[elapsed]
}
return { elapsed, polls, timedOut: polls >= 1_000 }
}
const wallClock = (elapsed) => elapsed + (elapsed >= 2_000 ? -10_000 : 0)
const monotonicClock = (elapsed) => elapsed
console.log(JSON.stringify({
wallClock: simulateDeadline(wallClock, {}),
monotonicClock: simulateDeadline(monotonicClock, {}),
nodeGlobalPerformance: typeof globalThis.performance?.now === 'function'
}))
JSRepository: stablyai/orca
Length of output: 304
🏁 Script executed:
#!/bin/bash
set -eu
node - <<'JS'
function simulateDeadline(clock) {
const deadline = clock(0) + 5_000
let elapsed = 0
let polls = 0
while (clock(elapsed) < deadline && polls < 1_000) {
elapsed += 50
polls += 1
}
return { elapsed, polls, timedOut: polls >= 1_000 }
}
console.log(JSON.stringify({
wallClock: simulateDeadline((elapsed) => elapsed + (elapsed >= 2_000 ? -10_000 : 0)),
monotonicClock: simulateDeadline((elapsed) => elapsed),
nodeGlobalPerformance: typeof globalThis.performance?.now === 'function'
}))
JSRepository: stablyai/orca
Length of output: 304
Use a monotonic deadline clock and update the fake-timer tests.
Date.now() can move backward and extend this five-second poll. Use performance.now() for both deadline calculations. Vitest 4.1.5 does not fake performance.now() by default, so include performance in the fake-timer setup and add coverage for clock changes.
| let stdout | ||
| try { | ||
| const result = await runCommand(command, [...prefixArgs, ...args, '--json'], { cwd }) | ||
| stdout = result.stdout | ||
| } catch (error) { | ||
| const parsed = JSON.parse(error?.stdout?.trim() ?? 'null') | ||
| if (parsed?.ok === false) { | ||
| throw Object.assign(new Error(parsed.error?.message ?? JSON.stringify(parsed.error)), { | ||
| code: parsed.error?.code | ||
| }) | ||
| } | ||
| throw error |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Preserve the original command failure when stdout is not JSON.
Line 84 throws SyntaxError when stdout is empty or contains non-JSON output. This hides the command error and its stderr. Catch JSON parsing failures and rethrow error.
Proposed fix
} catch (error) {
- const parsed = JSON.parse(error?.stdout?.trim() ?? 'null')
+ let parsed
+ try {
+ parsed = JSON.parse(error?.stdout?.trim() || 'null')
+ } catch {
+ throw error
+ }
if (parsed?.ok === false) {
throw Object.assign(new Error(parsed.error?.message ?? JSON.stringify(parsed.error)), {
code: parsed.error?.code
})📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let stdout | |
| try { | |
| const result = await runCommand(command, [...prefixArgs, ...args, '--json'], { cwd }) | |
| stdout = result.stdout | |
| } catch (error) { | |
| const parsed = JSON.parse(error?.stdout?.trim() ?? 'null') | |
| if (parsed?.ok === false) { | |
| throw Object.assign(new Error(parsed.error?.message ?? JSON.stringify(parsed.error)), { | |
| code: parsed.error?.code | |
| }) | |
| } | |
| throw error | |
| let stdout | |
| try { | |
| const result = await runCommand(command, [...prefixArgs, ...args, '--json'], { cwd }) | |
| stdout = result.stdout | |
| } catch (error) { | |
| let parsed | |
| try { | |
| parsed = JSON.parse(error?.stdout?.trim() || 'null') | |
| } catch { | |
| throw error | |
| } | |
| if (parsed?.ok === false) { | |
| throw Object.assign(new Error(parsed.error?.message ?? JSON.stringify(parsed.error)), { | |
| code: parsed.error?.code | |
| }) | |
| } | |
| throw error |
ELI5
Orca used to treat writing Enter into a terminal as proof that an agent received its prompt. This PR first waits for the Claude or Codex composer to finish rendering, presses Enter exactly once, and then waits for the agent lifecycle to show that the turn started or completed. If Orca cannot prove that within five seconds, it reports
agent_prompt_stalledand sends no more input.What Changed
agent_prompt_stalledafter five seconds without sending a recovery Enter.The guarded path is limited to recognized running Claude and Codex terminals. Other agents keep the existing terminal-send behavior.
Why
An extra Enter can submit a second command or interact with a different prompt. A visible, recoverable stall is safer than speculative input. Lifecycle acknowledgment gives Claude and Codex a bounded success signal without inspecting or matching composer contents.
Linked Issue
Fixes #14347
Visual Proof
N/A — this changes terminal input reliability and adds no UI. The rendered Orca terminal was still checked with real Claude and Codex sessions.
Testing
Validated on macOS:
pnpm run typecheck:nodepnpm run check:code-quality:changedpnpm run check:max-lines-ratchetpnpm run check:reliability-gatesLinux, Windows, and SSH were not manually exercised. The change is host-side, adds no wire fields or opcodes, and preserves the existing PTY/runtime abstraction for local, SSH, and folder workspaces.
Review
Three fresh
gpt-5.6-lunareviewers audited the exact pushed candidate across state-machine correctness, integration risk, and adversarial coverage. One review was clean. The overlapping concrete findings were fixed: fast working-to-idle cycles are retained explicitly, neutral title churn cannot impersonate a work cycle, newer working evidence clears older permission state, and permission appearing between paste chunks stops the remaining paste.Please focus on the one-Enter lifecycle acknowledgment, PTY-generation fencing, and partial-paste cleanup across process replacement.
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
There is no automatic recovery input. A stalled send returns a typed error so the caller or user can decide what to do next.
Checklist
N/Awith reasonpnpm lint,pnpm typecheck,pnpm test, andpnpm buildpass (targeted local validation is listed above; CI will run the full matrix)