fix(orchestration): scope idle mail nag to the bound Run - #13564
Open
bbingz wants to merge 1 commit into
Open
Conversation
Push-on-idle counted every undelivered row for the terminal handle, so a multi-Run coordinator saw other Runs' mail in \"You have N orchestration messages\". Filter terminal-handle nags to the pane's current bound Run. Fixes stablyai#13563.
Contributor
📝 WalkthroughWalkthroughUnread-message filtering now scopes non-Run mailboxes to the terminal pane’s bound Run. Reserved message types and messages with live waiters remain excluded. Test message construction accepts an optional 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: a005ab91-bb09-42d8-9400-e11349eb8597
📒 Files selected for processing (2)
src/main/runtime/orca-runtime.test.tssrc/main/runtime/orca-runtime.ts
Comment on lines
+34300
to
+34343
| it('scopes the idle mail pointer count to the pane bound Run only', async () => { | ||
| const runtime = new OrcaRuntimeService(store) | ||
| const db = new InMemoryOrchestrationMessages() | ||
| const write = vi.fn().mockReturnValue(true) | ||
| setInMemoryOrchestrationMessages(runtime, db) | ||
| runtime.setPtyController({ | ||
| write, | ||
| kill: vi.fn(), | ||
| getForegroundProcess: async () => null | ||
| }) | ||
| syncSinglePty(runtime) | ||
|
|
||
| const [terminal] = (await runtime.listTerminals()).terminals | ||
| db.setRun({ | ||
| id: 'run_bound', | ||
| coordinator_handle: terminal.handle, | ||
| coordinator_pane_key: 'tab-1:pane:1' | ||
| }) | ||
| runtime.onPtyData('pty-1', '\x1b]0;Codex done\x07', 101) | ||
| db.insertMessage({ | ||
| from: 'sender', | ||
| to: terminal.handle, | ||
| subject: 'other run', | ||
| runId: 'run_other' | ||
| }) | ||
| db.insertMessage({ | ||
| from: 'sender', | ||
| to: terminal.handle, | ||
| subject: 'bound run', | ||
| runId: 'run_bound' | ||
| }) | ||
|
|
||
| runtime.deliverPendingMessagesForHandle(terminal.handle) | ||
|
|
||
| expect(write).toHaveBeenCalledWith( | ||
| 'pty-1', | ||
| expect.stringContaining('You have 1 orchestration message') | ||
| ) | ||
| expect(write).not.toHaveBeenCalledWith( | ||
| 'pty-1', | ||
| expect.stringContaining('You have 2 orchestration messages') | ||
| ) | ||
| db.close() | ||
| }) |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Add regression cases for empty and unbound Runs.
This test covers one bound-Run message and one message from another Run. It does not verify that an empty bound Run produces zero messages or no nag. It also does not define or test behavior when the pane has no bound Run. Add focused cases for both conditions and assert the intended contract.
Comment on lines
+31958
to
+31979
| // Why: a pane with a bound Run must only nag for that Run's mail. Counting every | ||
| // undelivered row for the terminal handle included other coordinators' Runs (#13563). | ||
| const boundRun = this._orchestrationDb.getCurrentRunForPane?.(`${leaf.tabId}:${leaf.leafId}`) | ||
| const unread = this._orchestrationDb | ||
| .getUndeliveredUnreadMessages(mailboxHandle) | ||
| .filter( | ||
| (message) => | ||
| !options.reservedTypes?.has(message.type) && | ||
| !messageTypeHasLiveWaiter(waiters, message.type) | ||
| ) | ||
| .filter((message) => { | ||
| if (options.reservedTypes?.has(message.type)) { | ||
| return false | ||
| } | ||
| if (messageTypeHasLiveWaiter(waiters, message.type)) { | ||
| return false | ||
| } | ||
| if ( | ||
| boundRun && | ||
| !mailboxHandle.startsWith('run:') && | ||
| message.run_id && | ||
| message.run_id !== boundRun.id | ||
| ) { | ||
| return false | ||
| } | ||
| return true | ||
| }) |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Files matching orca-runtime:"
fd -i 'orca-runtime' . | sed 's#^\./##'
echo
echo "Git status concise:"
git status --short
echo
echo "Locate deliverPendingMessages and getCurrentRunForPane:"
rg -n "deliverPendingMessages|getCurrentRunForPane|getUndeliveredUnreadMessages|run_id|reservedTypes|Run" src/main/runtime/orca-runtime.ts | head -n 200
echo
echo "Segment around target lines:"
sed -n '31920,32010p' src/main/runtime/orca-runtime.ts | cat -n
echo
echo "Segment around getCurrentRunForPane definitions/usages in repository:"
rg -n "getCurrentRunForPane|getUndeliveredUnreadMessages|deliverPendingMessages" src -SRepository: stablyai/orca
Length of output: 12688
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Git status concise:"
git status --short
echo
echo "Locate target symbols:"
rg -n "deliverPendingMessages|getCurrentRunForPane|getUndeliveredUnreadMessages|run_id|reservedTypes|getCurrentRunForPane" src/main/runtime/orca-runtime.ts src/main/runtime/orca-runtime.test.ts src -S --glob '!src/main/runtime/orca-runtime.ts' | head -n 300
echo
echo "Segment around target lines in src/main/runtime/orca-runtime.ts:"
sed -n '31930,32000p' src/main/runtime/orca-runtime.ts | cat -n -v
echo
echo "Segment around getCurrentRunForPane definition if present:"
rg -n "getCurrentRunForPane|getUndeliveredUnreadMessages" src/main/runtime/orca-runtime.ts; sed -n '31600,31900p' src/main/runtime/orca-runtime.ts | cat -n -v
echo
echo "Behavioral probe: simulate filter from snippet with representative inputs"
python3 - <<'PY'
def filter_messages(messages, bound_run_id, mailbox):
def messageTypeHasLiveWaiter(waiters, msg_type):
# Placeholder: no live waiters in this probe.
return False
waiters = set()
return [
m for m in messages
if (
False if False else # options.reservedTypes placeholder
not messageTypeHasLiveWaiter(waiters, m["type"])
) and not (
bound_run_id and
not mailbox.startswith("run:") and
m.get("run_id") and
m["run_id"] != bound_run_id
)
]
messages = [
{"type": "mail", "run_id": "run-a"},
{"type": "mail", "run_id": "run-b"},
{"type": "mail"},
]
print("bound_run_id=run-a mailbox terminal:", filter_messages(messages, "run-a", "terminal:pane"))
print("bound_run_id=None mailbox terminal:", filter_messages(messages, None, "terminal:pane"))
print("bound_run_id=run-a mailbox run:", filter_messages(messages, "run-a", "run:mailbox"))
PYRepository: stablyai/orca
Length of output: 50371
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "target deliverPendingMessages implementation:"
sed -n '31919,32025p' src/main/runtime/orca-runtime.ts | cat -n -v
echo
echo "orchestration-db getCurrentRunForPane/usages in src/main:"
rg -n "getCurrentRunForPane|getUndeliveredUnreadMessages|deliverPendingMessagesForHandle|deliverPendingMessagesForLeaf" src/main --glob '*.ts' --glob '!src/main/runtime/orca-runtime.ts'
echo
sed -n '30820,30895p' src/main/runtime/orca-runtime.ts | cat -n -v
echo
echo "test helpers around messages/runs:"
sed -n '1028,1135p' src/main/runtime/orca-runtime.test.ts | cat -n -v
echo
echo "focused tests around no-bound-run / run scoping in orchestration tests:"
sed -n '34268,34355p' src/main/runtime/orca-runtime.test.ts | cat -n -v -s || true
sed -n '39970,40135p' src/main/runtime/orca-runtime.test.ts | cat -n -v -s @@ -40580,40635p src/main/runtime/orca-runtime.test.ts | cat -n -v -sRepository: stablyai/orca
Length of output: 28925
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "orchestration-current-run-for-pane tests:"
sed -n '34350,34400p' src/main/runtime/orca-runtime.test.ts | cat -n -v
echo
echo "run delivery getCurrentRunForPane tests:"
sed -n '235,285p' src/main/runtime/orchestration/orchestration-run-delivery-db.test.ts | cat -n -v
echo
echo "no-bound terminal deliverPendingMessages tests:"
python3 - <<'PY'
from pathlib import Path
p = Path("src/main/runtime/orca-runtime.test.ts")
text = p.read_text()
for i, line in enumerate(text.splitlines(), start=1):
if "deliverPendingMessagesForHandle" in line and i > 34330 and i < 34375:
print(f"\nNear context around line {i}:")
for j in range(max(1,i-25), min(len(text.splitlines()), i+30)+1):
print(f"{j:6}: {text.splitlines()[j-1]}")
PY
echo
echo "behavioral simulation of filter predicate:"
python3 - <<'PY'
messages = [
{"type": "status", "run_id": "run-other"},
{"type": "status"},
]
bound_run = None
mailbox_handle = "terminal:pane"
filtered = []
for message in messages:
excluded = False
if bound_run and not mailbox_handle.startswith("run:") and message.get("run_id") and message["run_id"] != bound_run["id"]:
excluded = True
if not excluded:
filtered.append(message)
print("bound_run:", bound_run)
print("input_count:", len(messages), "filtered_count:", len(filtered), "count:", len(filtered))
print("filtered_ids:", [m.get("run_id") for m in filtered])
PYRepository: stablyai/orca
Length of output: 6288
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
text = Path("src/main/runtime/orchestration/db.ts").read_text()
lines = text.splitlines()
for name, start in [("getCurrentRunForPane", 2440), ("getCurrentRunForPane", 2452), ("getUndeliveredUnreadMessages", 3490), ("getUndeliveredUnreadMessages", 3506)]:
print(f"\n{name} at line {start}\n")
for i in range(start, min(len(lines), start+120)+1):
print(f"{i:6}: {lines[i-1]}")
PYRepository: stablyai/orca
Length of output: 21620
Scope Run-tagged mail when no bound Run is available.
getCurrentRunForPane can return undefined for a pane, while getUndeliveredUnreadMessages still returns all undelivered rows for that handle. In that case, the filter does not exclude messages with another Run's run_id, so an unbound terminal can nag or wait for mail that belongs to a different Run. Skip those rows when boundRun is undefined too, unless the delivery contract explicitly supports DB-wide counting for unbound sessions.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
check --peekfor that Run was 0 (Bug: orchestration nag counts outside the session's bound Run #13563).run_id(Run mailboxes were already scoped viarun:<id>).Test plan
scopes the idle mail pointer count to the pane bound Run onlyFixes #13563