Repository navigation
fix(terminal): submit large agent-prompt pastes after TUI ingest (STA-2912) - #15698
brennanb2025 wants to merge 1 commit into
Conversation
📝 WalkthroughWalkthroughThe change adds Codex paste-ingestion detection and verified submission with bounded Enter retries. PTY relays maintain headless terminal emulators and expose synchronized snapshots with sequence and incarnation metadata. SSH providers negotiate and validate snapshot support. Prompt writes use centralized validation and settlement checks. Federated worker startup now requires the prompt-submit verification runtime capability. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 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: 1
🧹 Nitpick comments (2)
src/main/runtime/agent-prompt-submission-verification.ts (1)
3-4: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse one concise comment.
Replace the two-line comment with one line that states the Codex-specific reason.
Proposed fix
-// Why: framed paste and Enter must not share a PTY write; this is the same -// settle native chat uses between an image paste and the following submit. +// Keep paste and Enter separate so Codex does not absorb Enter as pasted content.As per coding guidelines, comments must be concise, non-obvious, and brief.
Source: Coding guidelines
src/main/runtime/agent-prompt-paste-submit-runtime.test.ts (1)
1-10: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse one platform-native worktree path across all fixtures.
Build the path with
join(tmpdir(), 'worktree-a')and reuse it in the mocked Git responses andruntime-rpc-worktree-store-fixtures.ts; changing onlyWORKTREE_PATHleaves mismatched selectors and metadata IDs on Windows.Source: Coding guidelines
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: ed77de03-24e8-4e89-ae1c-b14fbf243384
📒 Files selected for processing (12)
src/main/runtime/agent-prompt-paste-submit-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/orchestration/agent-prompt-send-accepted.tssrc/main/runtime/rpc/methods/orchestration-composed-workers.test.tssrc/main/runtime/rpc/methods/orchestration-federation.tssrc/main/runtime/rpc/methods/orchestration-tasks-dispatch.test.tssrc/main/runtime/rpc/methods/orchestration-workers.tssrc/main/runtime/rpc/methods/orchestration.tssrc/shared/agent-prompt-paste-blob.test.tssrc/shared/agent-prompt-paste-blob.ts
Included review availability: Your plan provides up to 10 included reviews per hour; 1 remains after this review.
| const combined = `${carry}${chunk}` | ||
| return { | ||
| detected: isUnsubmittedAgentPasteBlob(combined), | ||
| carry: combined.slice(-PASTE_BLOB_CARRY_MAX) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Prevent completed paste markers from remaining in parser state.
src/shared/agent-prompt-paste-blob.ts#L14-L17: clearcarryafter a complete marker or retain only an unmatched marker prefix.src/shared/agent-prompt-paste-blob.test.ts#L14-L19: add a marker-free follow-up chunk and assert thatdetectedisfalse.
📍 Affects 2 files
src/shared/agent-prompt-paste-blob.ts#L14-L17(this comment)src/shared/agent-prompt-paste-blob.test.ts#L14-L19
9d75eb5 to
70db5e8
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Actionable comments posted: 3
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 1a58e682-4390-465f-b6f8-2afc35103bb1
📒 Files selected for processing (18)
src/main/ipc/pty.tssrc/main/providers/pty-provider-contract.tssrc/main/providers/ssh-pty-provider-output-state.tssrc/main/providers/ssh-pty-provider.test.tssrc/main/providers/ssh-pty-provider.tssrc/main/providers/ssh-pty-visible-screen-snapshot.tssrc/main/runtime/agent-prompt-submission-runtime.test.tssrc/main/runtime/orca-runtime.tssrc/main/runtime/rpc/methods/orchestration-federated-worker-start.tssrc/main/runtime/rpc/methods/orchestration-migration-behavior.test.tssrc/main/runtime/rpc/methods/orchestration-prompt-submit-capability.tssrc/relay/pty-handler-output-streaming.test.tssrc/relay/pty-handler-spawn-admission.test.tssrc/relay/pty-handler.tssrc/shared/agent-prompt-codex-ingest.test.tssrc/shared/agent-prompt-codex-ingest.tssrc/shared/protocol-version.tssrc/shared/pty-visible-screen-snapshot.ts
Included review availability: Your plan provides up to 10 included reviews per hour; 5 remain after this review.
| canProvide(): boolean { | ||
| return this.supported | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Report capability only for the requested PTY.
IPtyProvider.canProvideAuthoritativeBufferSnapshot is an exact-PTY contract. After a successful host probe, the current code returns true for an unknown or exited PTY, but get() returns null because no incarnation is stored.
src/main/providers/ssh-pty-visible-screen-snapshot.ts#L16-L18: accept an app PTY ID and require a mapped relay PTY incarnation.src/main/providers/ssh-pty-provider.ts#L80-L81: passidtovisibleScreenSnapshots.canProvide(id).
Proposed fix
- canProvide(): boolean {
- return this.supported
+ canProvide(id: string): boolean {
+ return this.supported && this.readIncarnation(this.toRelayPtyId(id)) !== undefined
}- canProvideAuthoritativeBufferSnapshot = (_id: string): boolean =>
- this.visibleScreenSnapshots.canProvide()
+ canProvideAuthoritativeBufferSnapshot = (id: string): boolean =>
+ this.visibleScreenSnapshots.canProvide(id)📝 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.
| canProvide(): boolean { | |
| return this.supported | |
| } | |
| canProvide(id: string): boolean { | |
| return this.supported && this.readIncarnation(this.toRelayPtyId(id)) !== undefined | |
| } |
| canProvide(): boolean { | |
| return this.supported | |
| } | |
| canProvideAuthoritativeBufferSnapshot = (id: string): boolean => | |
| this.visibleScreenSnapshots.canProvide(id) |
📍 Affects 2 files
src/main/providers/ssh-pty-visible-screen-snapshot.ts#L16-L18(this comment)src/main/providers/ssh-pty-provider.ts#L80-L81
| private async readCodexPasteScreen( | ||
| ptyId: string, | ||
| generation: number | ||
| ): Promise<RuntimeVisibleTerminalState | null> { | ||
| if (!this.providerSnapshotPreferredPtys.has(ptyId)) { | ||
| const screen = await this.readHeadlessVisibleTerminalState(ptyId) | ||
| return screen?.generation === generation ? screen : null | ||
| } | ||
| const snapshot = await this.captureProviderTerminalBuffer( | ||
| ptyId, | ||
| { scrollbackRows: 0 }, | ||
| generation | ||
| ) | ||
| if ( | ||
| !snapshot || | ||
| this.getPtyLifecycleGeneration(ptyId) !== generation || | ||
| this.getPtyOutputSequence(ptyId) > snapshot.seq | ||
| ) { | ||
| return null | ||
| } | ||
| return { | ||
| lines: await this.parseVisibleSnapshotLines(snapshot), | ||
| isAlternateScreen: snapshot.alternateScreen ?? false, | ||
| sequence: snapshot.seq, | ||
| generation | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify every other provider-screen reader goes through the bounded wrapper.
rg -n -C4 'captureProviderTerminalBuffer|serializeProviderTerminalBuffer' src/main/runtime/orca-runtime.tsRepository: stablyai/orca
Length of output: 5345
🏁 Script executed:
#!/bin/bash
sed -n '13260,13325p;13520,13555p;19420,19680p' src/main/runtime/orca-runtime.tsRepository: stablyai/orca
Length of output: 12337
🏁 Script executed:
#!/bin/bash
rg -n -C6 'function withTimeout|const withTimeout|withTimeout\\(|VISIBLE_TERMINAL_SNAPSHOT_TIMEOUT_MS|readCodexPasteScreen\\(' src/main/runtime/orca-runtime.tsRepository: stablyai/orca
Length of output: 380
🏁 Script executed:
#!/bin/bash
rg -n -C6 -F \
-e 'function withTimeout' \
-e 'const withTimeout' \
-e 'withTimeout<' \
-e 'VISIBLE_TERMINAL_SNAPSHOT_TIMEOUT_MS' \
-e 'readCodexPasteScreen(' \
src/main/runtime/orca-runtime.tsRepository: stablyai/orca
Length of output: 5714
🏁 Script executed:
#!/bin/bash
rg -n -C5 -F 'function withTimeout' src
rg -n -C5 -F 'withTimeout = ' src
rg -n -C4 -F 'export function withTimeout' srcRepository: stablyai/orca
Length of output: 3838
🏁 Script executed:
#!/bin/bash
sed -n '1,40p' src/shared/promise-timeout-fallback.ts
sed -n '13317,13425p' src/main/runtime/orca-runtime.ts
rg -n -C4 -F 'assertAgentPromptRequestActive' src/main/runtime/orca-runtime.tsRepository: stablyai/orca
Length of output: 8797
Bound the provider snapshot read in readCodexPasteScreen.
captureProviderTerminalBuffer can await an unresponsive provider indefinitely. This bypasses the timeout used by serializeProviderTerminalBuffer, so the prompt poll and abort checks cannot complete while the read is pending. Route the read through serializeProviderTerminalBuffer with VISIBLE_TERMINAL_SNAPSHOT_TIMEOUT_MS, and make the read observe options.signal.
| export function countCodexPasteMarker(lines: readonly string[], marker: string): number { | ||
| let count = 0 | ||
| const visibleText = lines.join('') | ||
| let cursor = 0 | ||
| while (cursor <= visibleText.length - marker.length) { | ||
| const match = visibleText.indexOf(marker, cursor) | ||
| if (match === -1) { | ||
| break | ||
| } | ||
| count += 1 | ||
| cursor = match + marker.length |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- target implementation ---'
cat -n src/shared/agent-prompt-codex-ingest.ts | sed -n '1,100p'
printf '%s\n' '--- call sites ---'
rg -n --glob '*.{ts,tsx,js,jsx}' 'countCodexPasteMarker' .
printf '%s\n' '--- standalone behavior probe ---'
python3 - <<'PY'
def count(lines, marker, max_steps=1000):
visible_text = ''.join(lines)
cursor = 0
occurrences = 0
steps = 0
while cursor <= len(visible_text) - len(marker):
steps += 1
if steps > max_steps:
return ("non-terminating", occurrences, cursor)
match = visible_text.find(marker, cursor)
if match == -1:
break
occurrences += 1
cursor = match + len(marker)
return ("terminated", occurrences, cursor)
for lines, marker in [(["abc"], ""), (["abc"], "b"), ([""], ""), (["abc"], "z")]:
print(repr(lines), repr(marker), count(lines, marker))
PYRepository: stablyai/orca
Length of output: 3713
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- existing tests ---'
cat -n src/shared/agent-prompt-codex-ingest.test.ts | sed -n '55,95p'
printf '%s\n' '--- runtime caller ---'
cat -n src/main/runtime/orca-runtime.ts | sed -n '19600,19655p'
printf '%s\n' '--- ingest argument flow ---'
rg -n -C 4 --glob '*.{ts,tsx}' 'ingest\.marker|findCodexPasteMarkerDelta|collectCodexPasteMarkers' srcRepository: stablyai/orca
Length of output: 12848
Reject an empty marker before the scan.
When marker is empty, indexOf returns the current cursor and cursor does not advance. The loop does not terminate.
ELI5
Orca could paste a large task into Codex and press Enter before Codex had actually turned the paste into its
[Pasted Content N chars]composer item. The write succeeded, soworker-startreported a live worker even though the prompt was parked forever. This change waits for positive, delivery-owned screen evidence before submitting and refuses to report success when that evidence is unavailable.What changed
[Pasted Content N chars]delta before sending Enter.orchestration.prompt-submit-verification.v1before federatedworker-startcreates any home-side dispatch state.Why
The trigger is not a concatenated PTY write: paste and Enter were already separate. The failure is an ordering race under slow paste ingestion, and PTY quietness or cursor-show output does not prove that Codex owns the paste yet. The collapsed-paste marker is the positive signal available from Codex today.
Linked issue
Fixes STA-2912
Fixes #11343
Author: @BrennanKB5
Acceptance proof
One identical untracked black-box test was overlaid on three isolated checkouts. Test SHA-256:
17e61e73ba247bb0d70886254dfdf85bcd5f462b078408aeaa52575f2edf7257.3d74f5fe0315d8125b969704a8830a9a6f2ee9ba: failed becauseprematureEnterswas1before the marker appeared.70db5e8313b7cc6fa2543e1210e91332d47ab6db: passed.876e34c1ea6663fc19d70b87d0e4e1737b4ba731: failed on the sameprematureEnters === 0assertion with received value1.Validation
pnpm vitest run --config config/vitest.config.ts <15 affected files>— 1,389 passed, 1 skipped.pnpm typecheck— passed.node config/scripts/check-changed-code-quality.mjs— passed with 0 findings.node config/scripts/check-react-doctor-changed.mjs— passed with 0 issues.pnpm exec oxfmt --check <18 changed files>— passed.Limitations
[Pasted Content N chars]composer representation for large pastes; if Codex changes that representation, Orca fails closed instead of claiming the worker started.