Repository navigation
Conversation
📝 Walkthrough
Priority: ➖ Normal Severity of issue fixed: Medium Merge Risk: 🟡 Moderate · up to
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 75.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 8 functions across 13 files. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
-
Check name Status Explanation Title check The title clearly and concisely describes the primary change: preventing nested Claude quota failures from notifying the parent pane. Description check The description covers the required template sections, including the user impact, technical change, rationale, linked issue, visual proof, testing results, AI disclosure, review notes, compatibility c… Linked Issues check The PR satisfies the coding requirements in [ #30067].resolveAgentStatusIdentityandhasOtherAgentProcessOwnercompare PID, platform, and process-start identity before status state is inherited. T…Out of Scope Changes check The changes stay within [ #30067]. Source changes extend the existing process-owner guard and presence handling on the local and relay paths. Test changes verify the required admission and notification…Full details: Description check
-
Explanation
The description covers the required template sections, including the user impact, technical change, rationale, linked issue, visual proof, testing results, AI disclosure, review notes, compatibility considerations, and checklist status. It also clearly identifies tests that were not run.
Full details: Linked Issues check
-
Explanation
The PR satisfies the coding requirements in [
#30067].resolveAgentStatusIdentityandhasOtherAgentProcessOwnercompare PID, platform, and process-start identity before status state is inherited. The hook listener and relay pass the incoming identity before state admission. The execution-host presence check retains a live or unverifiable owner and admits a replacement after an exited owner. The local live-sender counter prevents an overlapping probe from declaring the pane exited. Added tests cover local and relay paths, delayed child retries, owner failures, unverifiable checks, replacement hooks, native subagent permission state, same-process changes, reused PIDs, and overlapping probes.
Full details: Out of Scope Changes check
-
Explanation
The changes stay within [
#30067]. Source changes extend the existing process-owner guard and presence handling on the local and relay paths. Test changes verify the required admission and notification behavior. The changes do not add unrelated provider handling, notification preferences, retry behavior, wire fields, or renderer ownership state.
-
- Fix all pre-merge checks with AI
- Autofix · 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.
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: 2
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Repository UI
- Review profile: CHILL
- Plan: Advanced
- Run ID:
fe1e22fd-c91e-4274-8d0a-e27e369c53a2
📒 Files selected for processing: 13
src/main/agent-hooks/server-agent-presence.test.tssrc/main/agent-hooks/server-claude-owed-notification-expiry.test.tssrc/main/agent-hooks/server-nested-claude-rate-limit.test.tssrc/main/agent-hooks/server/server-agent-presence.tssrc/main/agent-hooks/server/server-lifecycle.tssrc/main/agent-hooks/server/server-state.tssrc/main/agent-hooks/server/server-status-update.tssrc/shared/agent-hook-listener.tssrc/shared/agent-hook-owner-presence.tssrc/shared/agent-process-presence.tssrc/shared/agent-status-identity.tssrc/shared/agent-status-process-owner.test.tssrc/wsl-guest/agent-hook-event-admission.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.
| agentPresence: previousStatus.providerSessionOnly ? undefined : previousStatus.agentPresence | ||
| }, | ||
| incoming: source, | ||
| incomingProcess: agentProcess, |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🔎 Supported by static analysis
🏁 Script executed:
#!/bin/bash
# Inspect the relay cache writer and the hook normalization call order.
rg -n -C 8 'cacheRelayLegacyAgentStatus|hostEvidenceObservedAt|receivedAt' src --glob '*.ts'
rg -n -C 8 'normalizeHookPayload\s*\(|applyRelayHookEvent\s*\(' src/wsl-guest --glob '*.ts'Repository: stablyai/orca
Length of output: 45663
🏁 Script executed:
set -o pipefail
printf '%s\n' '--- symbol locations ---'
rg -n -F -- 'cacheRelayLegacyAgentStatus' src/wsl-guest src/shared
rg -n -F -- 'previousReceivedAt' src/shared src/wsl-guest
rg -n -F -- 'hostEvidenceObservedAt' src/shared src/wsl-guest
printf '%s\n' '--- provider normalization ---'
sed -n '1,130p' src/wsl-guest/agent-hook-provider-normalization.ts
printf '%s\n' '--- relay admission ---'
sed -n '1,180p' src/wsl-guest/agent-hook-event-admission.ts
printf '%s\n' '--- shared listener guard ---'
sed -n '130,225p' src/shared/agent-hook-listener.ts
printf '%s\n' '--- cache writer declaration ---'
rg -n -C 20 -F -- 'function cacheRelayLegacyAgentStatus' srcRepository: stablyai/orca
Length of output: 16646
🏁 Script executed:
set -o pipefail
rg -l -F -- 'function cacheRelayLegacyAgentStatus' src
rg -l -F -- 'cacheRelayLegacyAgentStatus =' src
rg -n -F -- 'cacheRelayLegacyAgentStatus' src/wsl-guest src/shared --glob '*.ts' | head -40
rg -n -F -- 'receivedAt:' src/wsl-guest/agent-hook* src/shared/agent-hook* --glob '*.ts' | head -80Repository: stablyai/orca
Length of output: 2327
🏁 Script executed:
set -o pipefail
printf '%s\n' '--- legacy admission helper ---'
rg -n -C 30 -F -- 'function admitLegacyAgentStatus' src/shared
printf '%s\n' '--- listener event type ---'
sed -n '1,115p' src/shared/agent-hook-listener/listener-event.ts
printf '%s\n' '--- listener state and admission types ---'
rg -n -C 18 -F -- 'lastStatusByPaneKey' src/shared/agent-hook-listener/listener-state.ts
rg -n -C 25 -F -- 'export function admitLegacyAgentStatus' src/shared/agent-hook-listener/listener-state.tsRepository: stablyai/orca
Length of output: 19483
🏁 Script executed:
set -o pipefail
printf '%s\n' '--- adapter definitions ---'
rg -n -C 25 -F -- 'createAgentStatusLegacyAdapter' src/shared
rg -n -C 25 -F -- 'admit(caller' src/shared
rg -n -C 25 -F -- 'class AgentStatusLegacy' src/shared
printf '%s\n' '--- adapter files ---'
rg --files src/shared | rg 'legacy|status.*adapter'Repository: stablyai/orca
Length of output: 32743
🏁 Script executed:
set -o pipefail
sed -n '1,280p' src/shared/agent-status-legacy-adapter.ts
rg -n -C 20 -F -- 'admit(' src/shared/agent-status-legacy-adapter.tsRepository: stablyai/orca
Length of output: 8358
🏁 Script executed:
set -o pipefail
rg -n -C 12 -F -- 'hostEvidenceObservedAt' src/shared/agent-hook-listener.ts src/shared/agent-hook-listener --glob '*.ts'
rg -n -C 18 -F -- 'return {' src/shared/agent-hook-listener.ts | tail -160
rg -n -C 10 -F -- 'normalizeHookPayload' src/shared/agent-hook-listener.tsRepository: stablyai/orca
Length of output: 8273
🏁 Script executed:
sed -n '291,345p' src/shared/agent-hook-listener.tsRepository: stablyai/orca
Length of output: 1809
Run the relay ownership guard without a timestamp prerequisite.
cacheRelayLegacyAgentStatus stores the normalized event unchanged. Relay events can lack both receivedAt and hostEvidenceObservedAt. In that case, previousReceivedAt is undefined and the process-owner comparison is skipped. RelayProviderHookNormalization can then copy prompt and tool caches before applyRelayHookEvent rejects the foreign hook.
Move the process guard before the timestamp-dependent condition, or ensure every normalized relay event receives one of these timestamps before caching.
| (presence.agent !== incoming || | ||
| (incomingProcess && !isSameAgentProcess(presence.process, incomingProcess))) && |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Fence a different Claude process when the owner is suspended.
If a probe leaves the recorded owner marked suspended: true, the preceding !presence.suspended check disables this new process comparison. A different Claude process can then pass the same-agent fast path in src/shared/agent-status-identity.ts and the relay guard in src/wsl-guest/agent-hook-event-admission.ts, even when the owner has not exited. Its prompt or failure can replace the owner's state. Keep a mismatched process fenced until the execution host proves that the owner exited, and align the resolver's suspended branch. The PR objective requires a live or unverifiable owner to retain the pane.
ELI5
A background
claude -pretry can currently make the phone say the interactive Claude failed every time the child hits its usage limit. Keep the interactive session’s status intact while its process still owns the pane; its own genuine failure still notifies.What Changed
Extend the existing process-owner guard to distinguish two Claude processes, using the existing PID, platform and process-start identity. Reject a different process before folding its prompts, native subagents or turn state, on both the local hook server and the SSH/WSL relay.
Reuse the existing execution-host presence check before admission. A
liveorunverifiableowner keeps the pane, even when idle or its last hook is old. Anexitedowner yields to the first replacement hook. The local HTTP path also reuses the existing live-sender counter so an overlapping presence read does not mistake replacement of the old owner for exit of the entire pane.Why
The current agent-type guard accepts both producers because both say “Claude.” Extending that guard is smaller than a new ownership subsystem and protects all parent state, rather than hiding notifications with a cooldown or globally muting real failures. This is a focused alternative pending the broader ownership work in #26408, related to #26020; it does not close the broader legacy and cross-provider report.
Linked Issue
Fixes #30067
Visual Proof
N/A — no layout or controls change. Reproducible before/after evidence comes from real loopback hook requests in
server-nested-claude-rate-limit.test.ts, through both a local server and a relay forwarding into the desktop:StopFailure: rate_limit, 15 minutes apartThe first sequence failed against the base revision before the fix. Tests count host status broadcasts, not APNs deliveries. The issue separately records the observed iOS banners and correlated host notification records.
Testing
ORCA_BACKGROUND_LAUNCH=1 pnpm test:node src/main/agent-hooks src/shared/agent-status-process-owner.test.ts src/shared/agent-hook-listener*.test.ts src/wsl-guest/agent-hook-server*.test.ts: 1,839 passed, 55 skipped; 168 files passed, 1 skipped.ORCA_BACKGROUND_LAUNCH=1 pnpm tc: passed.pnpm run check:code-quality:changed 6fcc183ab951fb751cf02ccb9e2af3cf129a4316: passed, zero new findings across 13 files.Added coverage for working/idle owners, 16 delayed retries, unverifiable owners, first-hook replacement, native subagent permission preservation, same-process
/clear, reused PIDs, and overlapping presence reads. The old owed-notification fixture now explicitly mocks its fictional owner process as live.Actual runtime: macOS, real local HTTP and local relay-to-desktop HTTP. Process probing is mocked. Pure identity tests exercise macOS/Linux/Windows platform values; no live Linux, Windows, SSH host, real Claude CLI or phone push test was run. No GUI or installed-app modification.
test:nodewas used because Bun is unavailable.I manually tested these changes locally
Automated tests added/updated, or explained why not below
AI Disclosure
Implemented and checked with OpenAI Codex.
Review
Self-reviewed the early rejection boundary, host-owned liveness decisions, session changes within one process, and genuine owner failure path. No quota-reset, retry-script or notification-preference changes.
Agent skill upstream boundary
Notes
Requires hooks that include process identity. Legacy hooks and current Windows hooks without it retain their existing behavior. No wire fields, new process enumeration or renderer-side ownership cache are added. Remote decisions stay on the execution host; disconnect/unverifiable is not treated as process exit. Folder workspaces use the same pane guard. The existing presence check deduplicates concurrent probes; each separately arriving foreign process hook may trigger another check.
Checklist
N/Awith reasonpnpm lint,pnpm typecheck,pnpm test, andpnpm buildpass (or CI will cover; local preferred)Full lint/build were not run; the actual local checks are listed above.