Skip to content

fix(omp): recover retired pane status with validated restart authority - #21675

Merged
nwparker merged 5 commits into
mainfrom
nwparker/omp-retired-pane-recovery-current
Sep 19, 2026
Merged

nwparker merged 5 commits into
mainfrom
nwparker/omp-retired-pane-recovery-current

Conversation

@nwparker

@nwparker nwparker commented Sep 19, 2026 •

Copy link
Copy Markdown
Contributor
Files Added Deleted Net
Test 9 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​489 $\color{#cf222e}{\Huge{\mathbf{−}}}$​9 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​480
Prod 29 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​727 $\color{#cf222e}{\Huge{\mathbf{−}}}$​345 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​382

ELI5

Orca keeps a small “this pane has retired” note so stale OMP messages cannot resurrect a closed terminal. After a daemon-backed restart, a live OMP turn could be mistaken for one of those stale messages, so the sidebar stayed idle forever. This change gives each renderer retirement a one-time receipt. The host only sends that receipt after validating a real OMP turn, and the renderer clears only the matching note. Closed tabs, replays, malformed relay data, and old receipts stay blocked.

Problem

An OMP process can survive an Orca main/renderer reconnect or reuse a shell pane after command completion. The old pane authority fence then suppresses the next before_agent_start/agent_end status sequence. The OMP process is still healthy, but Orca loses its working/done state and completion indicator.

What changed

  • Validate remote hook metadata and payload before changing retirement state.
  • Recognize source-less legacy OMP relays from the validated agentType: "omp" payload.
  • Carry a one-shot, renderer-generated retirement UUID through the trusted main-to-renderer status event.
  • Restore the whole retired alias group only for the matching live OMP restart; preserve stronger closed-tab fences.
  • Keep restart authority out of status persistence, hydration, replay, and cached snapshots.
  • Preserve remote/SSH compatibility: source remains optional and no new relay opcode or required field is introduced.

Scope and compatibility

  • Desktop/main, preload, renderer, and SSH relay status paths only.
  • No OMP source changes. The read-only reference checkout is /Users/nwparker/projects/oh-my-pi.
  • OMP’s current extension emits before_agent_start, agent_start, and agent_end; the source-aware boundary classifier now admits the first event after a retired-pane fence.
  • Closed tabs remain permanently suppressed, including after tab tombstone LRU eviction.
  • No UI layout, styling, or mobile changes.

Visual proof

N/A — this is a status-authority transition with no layout change. The visible result is the existing OMP spinner/completion state receiving the authoritative status row again. Runtime proof below exercises the real OMP extension and the host/renderer lifecycle; the renderer unit harness verifies that the recovered row is accepted and a replay is not.

Verification

  • ORCA_BACKGROUND_LAUNCH=1 pnpm test src/main/agent-hooks src/main/ipc src/main/ssh/ssh-relay-session-agent-hooks.integration.test.ts src/renderer/src/store/slices src/renderer/src/hooks/ipc-events — 8,299 passed, 20 skipped (860 files, 1 skipped).
  • ORCA_BACKGROUND_LAUNCH=1 pnpm test on all nine changed lifecycle suites — 90 passed.
  • pnpm tc:node — passed.
  • pnpm tc:web — passed.
  • pnpm run check:code-quality:changed -- origin/main — 0 findings across 34 changed files.
  • git diff --check and oxfmt --check on all 35 changed files — passed.
  • bun tests/tools/omp-child-session-runtime-smoke.mjs /Users/nwparker/projects/oh-my-pi — actual OMP loader, ExtensionRunner, SessionManager, and native HTTP; 8 expected lifecycle posts, child sessions quiet.
  • bun tests/tools/omp-completion-runtime-smoke.mjs — generated OMP extension with real native HTTP 503 delivery and retry cancellation; passed.
  • The rebased branch is based directly on current origin/main (b0ec11f5b0b8) and has no unrelated commits.

Readiness checklist

  • ELI5 included.
  • Focused regression tests cover local and remote OMP ingress, malformed metadata, replay, alias transfer, repeated retirement, renderer batching, and closed-tab suppression.
  • Runtime proof uses the actual OMP source checkout and generated extension.
  • Typecheck, changed-file quality, formatting, and broad relevant test suites pass.
  • Cross-platform and SSH/relay behavior considered; wire additions are optional and additive.
  • No negative user-facing trade-off identified; ordinary live status handling is unchanged.
  • Full desktop packaging and hosted CI remain merge gates.

Merge risk

Medium-low. The fix crosses the existing main/preload/renderer authority path because the bug has independent host and renderer fences. Changes are additive, bounded, and guarded by focused tests; no persisted-state migration or wire-version requirement is introduced. Hosted CI and review should confirm the rebased stack before merge.

Related work

This supersedes the stale-stack implementation in #20675 and complements merged #14624. It fixes #14236. Existing #14560 remains a separate, turn-triggered recovery approach; this branch keeps authoritative reattach recovery as the primary contract and does not depend on that PR.

AI disclosure

OpenAI Codex was used to rebase, review, test, and prepare this replacement. The implementation retains the original authorship and co-author attribution from #20675.

nwparker and others added 3 commits September 19, 2026 06:00
Co-authored-by: euclodius <244837876+euclodius@users.noreply.github.com>
Co-authored-by: euclodius <244837876+euclodius@users.noreply.github.com>
@nwparker

Copy link
Copy Markdown
Contributor Author

Local readiness update (current-main replacement):

  • Full relevant suites: 8,299 passed, 20 skipped across 860 files.
  • Nine changed lifecycle suites: 90 passed.
  • pnpm tc:node and tc:web passed.
  • Changed-code quality: 0 findings across 34 files.
  • oxfmt/diff-check passed.
  • Actual OMP source runtime: bun tests/tools/omp-child-session-runtime-smoke.mjs /Users/nwparker/projects/oh-my-pi passed (8 expected lifecycle posts). Generated OMP extension completion retry smoke passed.

Hosted checks are still pending; no merge claim until they are green.

@nwparker

Copy link
Copy Markdown
Contributor Author

Hosted static analysis reports the same pre-existing base errors as the current-main image PR: src/main/agent-hooks/server/server-status-update.ts (304 lines) and src/main/agent-hooks/server/server-ingest-remote.ts (305 lines). The rebased change itself has zero changed-code findings locally; this is a repository-wide baseline gate failure, not a finding in the 34 changed files.

@coderabbitai

coderabbitai Bot commented Sep 19, 2026 •

Copy link
Copy Markdown
Contributor

Review Change StackReview Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Advanced

Run ID: df7625d1-0bfe-4a4b-80e9-5cb30178146e

📥 Commits

Reviewing files that changed from the base of the PR and between 602b498 and b34ca86.

📒 Files selected for processing (1)
  • src/shared/agent-status-legacy-ingress-manifest.ts

Included review availability: Your plan provides up to 10 included reviews per hour; 8 remain after this review.


📝 Walkthrough

Walkthrough

The change adds retirement UUIDs to pane-authority IPC and hook-server fences. Validated live OMP restart events consume matching IDs and emit authorityRestartId only on live notifications. Renderer state matches this ID before clearing retirement tombstones and restores aliases for detached panes. Closed tabs remain suppressed. Remote metadata validation rejects malformed replay and launch-token values. Tests cover ingress, IPC, server recovery, renderer routing, LRU eviction, and detached-pane cases.

Priority: ➖ Normal

Severity of issue fixed: Medium

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (2 warnings)

Check name Status Explanation Resolution
Linked Issues check ⚠️ Warning Issue #14236 requires valid OMP/Pi reattach recovery, restoration of main and renderer authority, closed-tab and stale-event suppression, pane and turn scoping, and regression coverage for the `workin… Add a reattach regression test that applies the matching live OMP new-turn event as working, applies the matching non-replay completion event as done, and asserts the final done state exactly once in main and renderer paths. Also asse…
Docstring Coverage ⚠️ Warning Docstring coverage is 25.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 24 functions across 37 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly identifies the primary change: recovering retired OMP pane status with validated restart authority.
Description check ✅ Passed The description is detailed and covers the user impact, implementation, rationale, linked issue, visual proof, testing, compatibility, AI disclosure, and merge risk. It does not use every template hea…
Out of Scope Changes check ✅ Passed The changes stay within Issue #14236. Retirement UUID correlation, OMP and relay validation, renderer ownership checks, persistence exclusions, alias recovery, closed-tab fences, and focused tests sup…
Full details: Linked Issues check

Explanation

Issue #14236 requires valid OMP/Pi reattach recovery, restoration of main and renderer authority, closed-tab and stale-event suppression, pane and turn scoping, and regression coverage for the working to done lifecycle. The implementation validates OMP metadata, carries a retirement UUID, restores the active pane, rejects replay and closed-tab recovery, and clears the renderer tombstone only for the matching authority. The added host-renderer tests verify recovery to working and stale-event protection. They do not ingest an accepted completion event or assert a single working to done transition in main and renderer state.

Resolution

Add a reattach regression test that applies the matching live OMP new-turn event as working, applies the matching non-replay completion event as done, and asserts the final done state exactly once in main and renderer paths. Also assert that replayed or stale completion events do not change the state.

  • Fix all pre-merge checks with AI
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Commit to this branch
  • Create a new PR

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to GitHub limitations.

⚠️ Outside diff range comments (1)

🟠 Major · Do not restore a closed retirement fence. · server-authority-fences.ts:118-120

src/main/agent-hooks/server/server-authority-fences.ts:118-120
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Do not restore a closed retirement fence.

If a fence spans tabs and one member tab closes, an attach through another open member can resolve the fence and remove it. Later stale OMP events for the open member may then bypass fence suppression.

    const fence =
      this.retiredPaneFencesByKey.get(paneKey) ?? this.retiredPaneFencesByKey.get(ownerPaneKey)
    if (fence?.closed) {
      return false
    }

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Advanced

Run ID: ba213014-1f55-46f8-9d76-f4002151add0

📥 Commits

Reviewing files that changed from the base of the PR and between b0ec11f and 911db95.

📒 Files selected for processing (35)
  • docs/reference/agent-status-store.md
  • src/main/agent-hooks/server-omp-retired-ingress.test.ts
  • src/main/agent-hooks/server-omp-retirement-ack.test.ts
  • src/main/agent-hooks/server/server-authority-aliases.ts
  • src/main/agent-hooks/server/server-authority-fences.ts
  • src/main/agent-hooks/server/server-ingest-remote.ts
  • src/main/agent-hooks/server/server-lifecycle.ts
  • src/main/agent-hooks/server/server-state.ts
  • src/main/agent-hooks/server/server-status-disposition.ts
  • src/main/agent-hooks/server/server-status-update.ts
  • src/main/agent-hooks/server/server-types.ts
  • src/main/ipc/agent-hooks.test.ts
  • src/main/ipc/agent-pane-authority-ipc.ts
  • src/main/ipc/agent-pane-retirement-ipc.test.ts
  • src/main/ssh/ssh-relay-session-agent-hooks.integration.test.ts
  • src/main/ssh/ssh-relay-session.ts
  • src/main/startup/main-window-agent-status.ts
  • src/main/startup/main-window-structured-status-filter.test.ts
  • src/preload/api/agent-status-api.ts
  • src/preload/api/agent-status-bridge.ts
  • src/renderer/src/components/dashboard/useRetainedAgents.ts
  • src/renderer/src/hooks/ipc-events/agent-status-event-applicator.ts
  • src/renderer/src/hooks/ipc-events/agent-status-routing.ts
  • src/renderer/src/hooks/ipc-events/normalize-agent-status-event.ts
  • src/renderer/src/lib/agent-status-pane-ownership.ts
  • src/renderer/src/store/slices/agent-pane-authority.test.ts
  • src/renderer/src/store/slices/agent-status-authority-actions.ts
  • src/renderer/src/store/slices/agent-status-contract.ts
  • src/renderer/src/store/slices/agent-status-drop-reducer.ts
  • src/renderer/src/store/slices/agent-status-live-actions.ts
  • src/renderer/src/store/slices/agent-status-pane-keyed-records.ts
  • src/renderer/src/store/slices/agent-status-slice-contract.ts
  • src/renderer/src/store/slices/terminal-pane-detach-agent-retention.test.ts
  • src/shared/agent-status-ipc-payload.ts
  • tests/e2e/omp-retired-host-renderer.unit.test.ts

Included review availability: Your plan provides up to 10 included reviews per hour; 5 remain after this review.

Comment on lines +95 to +99
closedAgentStatusRetirementKeys(
s.recentlyRetiredAgentStatusPaneKeys,
prefix,
retiredAliasPaneKeys
)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

🔎 Supported by static analysis

🏁 Script executed:

sed -n '1,180p' src/renderer/src/store/slices/agent-status-drop-reducer.ts
sed -n '110,165p' src/renderer/src/store/slices/agent-status-pane-keyed-records.ts
rg -n 'dropAgentStatusByTabPrefix|paneKeys:' src/renderer/src/store tests/e2e

Repository: stablyai/orca

Length of output: 14493


🏁 Script executed:

sed -n '80,155p' src/renderer/src/store/slices/agent-status-drop-reducer.ts
sed -n '1,155p' src/renderer/src/store/slices/agent-status-drop-actions.ts
sed -n '1,150p' src/renderer/src/store/slices/agent-status-pane-keyed-records.ts
sed -n '1,115p' src/renderer/src/store/slices/retired-terminal-tab-state-sweep.ts
rg -n -C 5 'recentlyRetiredAgentStatusPaneKeys|closedAgentStatusRetirementKeys|retiredAliasPaneKeys|retire|permanent|restart|OMP' src/renderer/src/store/slices tests/e2e/omp-retired-host-renderer.unit.test.ts

Repository: stablyai/orca

Length of output: 50370


🏁 Script executed:

printf '%s\n' '--- reducer ---'
nl -ba src/renderer/src/store/slices/agent-status-drop-reducer.ts | sed -n '40,112p'
printf '%s\n' '--- scoped sweep ---'
nl -ba src/renderer/src/store/slices/retired-terminal-tab-state-sweep.ts | sed -n '1,105p'
printf '%s\n' '--- restore path ---'
nl -ba src/renderer/src/store/slices/agent-status-authority-actions.ts | sed -n '137,175p'
printf '%s\n' '--- retirement helper ---'
nl -ba src/renderer/src/store/slices/agent-status-pane-keyed-records.ts | sed -n '82,135p'
printf '%s\n' '--- scoped drop callers ---'
rg -n -C 6 --glob '*.ts' --glob '*.tsx' 'dropAgentStatusByTabPrefix\([^)]*\{[^}]*paneKeys|paneKeys.*dropAgentStatusByTabPrefix|dropAgentStatusByTabPrefix' src/renderer/src/store/slices/retired-terminal-tab-state-sweep.ts src/renderer/src/store/terminals/terminal-shutdown.ts src/renderer/src/store

Repository: stablyai/orca

Length of output: 47261


🏁 Script executed:

printf '%s\n' '--- retirement bounds and closure helper ---'
nl -ba src/renderer/src/store/slices/agent-status-pane-keyed-records.ts | sed -n '1,82p'
nl -ba src/renderer/src/store/slices/agent-status-pane-keyed-records.ts | sed -n '127,155p'
printf '%s\n' '--- drop action ---'
nl -ba src/renderer/src/store/slices/agent-status-drop-actions.ts | sed -n '118,150p'
printf '%s\n' '--- restore callers ---'
rg -n -C 8 --glob '*.ts' --glob '*.tsx' 'restoreAgentPaneAuthority' src/renderer/src

Repository: stablyai/orca

Length of output: 28147


🏁 Script executed:

rg -n -C 5 --glob '*.ts' --glob '*.tsx' "recentlyRetiredAgentStatusPaneKeys|retirementId|=== true|typeof .*recentlyRetired" src/renderer/src/store/slices src/renderer/src/components/terminal-pane | head -n 260

Repository: stablyai/orca

Length of output: 26275


Limit retirement closure to the selected panes.

When opts.paneKeys is set, closedAgentStatusRetirementKeys still scans every retired key under prefix. boundRecentlyRetiredAgentStatusPaneKeys then rewrites unrelated sibling entries to true.

agent-status-live-actions.ts rejects status updates for a retired key whose value is not the matching authorityRestartId. A matching restart status for a sibling is therefore dropped until its PTY reattach calls restoreAgentPaneAuthority.

Use the tab prefix only for full-tab drops. For pane-scoped drops, seed the expansion with opts.paneKeys and aliases for those panes only.

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ℹ️ No critical issues — a few coverage gaps and one latent host/renderer mismatch, all inline.

Reviewed changes

  • Host retirement ids: retirePaneAuthority now stores an optional renderer-generated UUID in RetiredPaneFence.retirementIdsByPaneKey; restoreRetiredStatusRestart consumes it on a validated OMP new turn, lifts the fence, and the live emit carries authorityRestartId only when not a replay.
  • Ingress validation: remote and SSH relay ingress reject malformed isReplay/launchToken, OMP source/agentType mismatch, and malformed providerSessionOnly; effectiveSource reconstructs omp provenance for source-less legacy relays.
  • Closed-tab fences: closing a tab records a per-pane fence, marks the whole alias group closed and clears its ids; restorePaneAuthority/restoreRetiredPaneFence gained fence-identity guards.
  • Renderer tombstones: recentlyRetiredAgentStatusPaneKeys values are now true | string; retireAgentPaneAuthority mints a UUID, propagates it across the retired alias group, and the live-status path clears only the matching tombstone after owner/agentType/workspace/connection validation.
  • Ownership refactor: resolvePaneKey/resolveWorktreeConnection moved to lib/agent-status-pane-ownership.ts using the indexed worktree/repo maps (behaviour-preserving).

Pullfrog  | Fix all ➔ | Fix 👍s ➔ | View workflow run | Using DeepSeek Flash (free via Pullfrog for OSS) | 𝕏

Comment on lines +65 to +90
const apply = (data: AgentStatusIpcPayload, replay = false, batch = false) => {
const id = replay ? undefined : data.authorityRestartId
if (isAgentStatusForRecentlyClosedTab(store.getState(), data.paneKey, id)) {
return
}
const update = {
paneKey: data.paneKey,
payload: data,
timing: { updatedAt: data.receivedAt, stateStartedAt: data.stateStartedAt },
metadata: id ? { authorityRestartId: id } : undefined
}
if (batch) {
store.getState().setAgentStatuses([update])
} else {
store
.getState()
.setAgentStatus(
update.paneKey,
update.payload,
undefined,
update.timing,
undefined,
update.metadata
)
}
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This apply helper reimplements createAgentStatusEventApplicator — it hand-builds metadata and calls isAgentStatusForRecentlyClosedTab/setAgentStatus directly — so the production glue that carries authorityRestartId is never exercised. A regression in the applicator's agentType === 'omp'/replay gate, its !authorityRestartId ownership fallback, or in normalizeAgentStatusMetadata would ship green. Driving the real applicator (or at least normalizeAgentStatusMetadata) here would pin it.

Technical details
# Renderer acknowledgement glue has no failing-capable test

## Affected sites
- tests/e2e/omp-retired-host-renderer.unit.test.ts:65-90 — hand-rolled `apply` bypasses the applicator
- src/renderer/src/hooks/ipc-events/agent-status-event-applicator.ts:56-58,88-93,251 — untested
- src/renderer/src/hooks/ipc-events/normalize-agent-status-event.ts:29-41 — no test references
- src/renderer/src/store/slices/agent-status-live-actions.ts:83-95 — owner/agentType/routing rejections untested (the helper always passes OMP payloads with `routing: undefined`, so those branches short-circuit)

## Required outcome
- The recovery ack is covered through `createAgentStatusEventApplicator` (or at minimum `normalizeAgentStatusMetadata`), so removing the `agentType === 'omp'` gate, the metadata branch, or the owner/routing validation fails a test.

Comment on lines +109 to +124
const restartedAuthority =
statusDisposition === 'restart' && source === 'omp'
? this.restoreRetiredStatusRestart(normalized.event.paneKey)
: undefined
const event =
statusDisposition === 'restart'
? { ...normalized.event, launchToken: undefined }
? {
...normalized.event,
launchToken: undefined,
...(restartedAuthority
? {
...restartedAuthority,
tabId: parsePaneKey(restartedAuthority.paneKey)?.tabId
}
: {})
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The local HTTP ingress is the only other place that injects restartedAuthority into an event, but no test drives it — every acknowledgement test uses ingestRemote. Deleting this block would leave the suite green while local OMP panes silently stop recovering. A retirePaneAuthority(PANE, ID) plus a local OMP before_agent_start POST asserting the emitted authorityRestartId would close the gap.

Technical details
# Local acknowledgement emission untested

## Affected sites
- src/main/agent-hooks/server/server-lifecycle.ts:109-124 — the local-path ack injection
- src/main/agent-hooks/server-omp-retirement-ack.test.ts — all cases use `ingestRemote`
- src/main/agent-hooks/server-omp-retired-ingress.test.ts — all cases use `ingestRemote`

## Required outcome
- A test that retires with an id and posts a local OMP new-turn event asserts the live listener receives `authorityRestartId`, and that a replay does not.

Comment on lines +111 to +118
retiredPaneKeys,
s.recentlyRetiredAgentStatusPaneKeys[ownerPaneKey] === true ||
isRecentlyClosedAgentStatusTab(
s.recentlyClosedAgentStatusTabIds,
getTabIdFromPaneKey(ownerPaneKey)
)
? true
: retirementId

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The renderer decides a retirement is permanently closed from the owner key alone, while the host computes closed across every key in the fence group. When closure reaches the group through a physical alias, the host stores closed: true with no id but the renderer stores a string id and sends it into a void, contradicting the invariant the new docs state ("Closed tombstones remain true"). I could not construct live-status loss from this, but the two sides should classify a group as closed from the same key set.

Technical details
# Closed computation disagrees across host and renderer

## Affected sites
- src/renderer/src/store/slices/agent-status-authority-actions.ts:111-118 — closed is derived from `s.recentlyRetiredAgentStatusPaneKeys[ownerPaneKey] === true` and the owner tab only
- src/main/agent-hooks/server/server-status-disposition.ts:176-179 — `closed` is true when ANY key in `paneKeys` is closed
- docs/reference/agent-status-store.md:367-371 — documents "Closed tombstones remain `true`"

## Required outcome
- Both sides classify a retired group as closed using the same key set, so a group closed via a physical alias is represented as `true` (and no id is sent) on the renderer too.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1


ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Advanced

Run ID: 2ec84ec5-70bd-45e0-a350-9f6d44d6caf3

📥 Commits

Reviewing files that changed from the base of the PR and between 911db95 and 602b498.

📒 Files selected for processing (4)
  • src/main/agent-hooks/server/server-ingest-remote.ts
  • src/main/agent-hooks/server/server-remote-envelope-normalization.ts
  • src/main/agent-hooks/server/server-status-application.ts
  • src/main/agent-hooks/server/server-status-update.ts
💤 Files with no reviewable changes (1)
  • src/main/agent-hooks/server/server-status-update.ts

Included review availability: Your plan provides up to 10 included reviews per hour; 8 remain after this review.

Comment on lines +36 to +44
}
const {
receivedAt: _receivedAt,
evidenceObservedAt: _evidenceObservedAt,
stateStartedAt,
observation: _observation,
restoredUnconfirmed: _restoredUnconfirmed,
isReplay: _isReplay,
...payload

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win

🔎 Supported by static analysis

🏁 Script executed:

sed -n '1,125p' src/main/agent-hooks/server/server-status-application.ts
sed -n '1,290p' src/main/agent-hooks/server/server-status-update.ts
rg -n -C 4 'refreshTerminalStatusEvidence|authorityRestartId|PersistedAgentHookEventPayload|writeLegacyStatusRow|admitLegacyAgentStatus' src/main/agent-hooks/server src/main

Repository: stablyai/orca

Length of output: 50369


🏁 Script executed:

#!/bin/bash
set -eu
printf '%s\n' '--- listener-state bindings ---'
rg -n -C 18 'function admitLegacyAgentStatus|const admitLegacyAgentStatus|admitLegacyAgentStatus\s*=|function getStatusSnapshot|getStatusSnapshot\s*\(' src/shared/agent-hook-listener/listener-state.ts src/main/agent-hooks/server src/main 2>/dev/null | head -n 260
printf '%s\n' '--- persistence serializer ---'
sed -n '15,75p' src/main/agent-hooks/server/server-persistence.ts
printf '%s\n' '--- enriched emission bindings ---'
rg -n -C 12 'emitEnrichedStatus\s*\(' src/main/agent-hooks/server src/main | head -n 220
printf '%s\n' '--- focused tests ---'
rg -n -C 12 'refreshTerminalStatusEvidence|authorityRestartId.*(snapshot|persist|refresh)|getStatusSnapshot\(\).*authorityRestartId|not\.toHaveProperty.*authorityRestartId' src/main/agent-hooks --glob '*test*' | head -n 260

Repository: stablyai/orca

Length of output: 50369


🏁 Script executed:

#!/bin/bash
set -eu
printf '%s\n' '--- listener-state imports and adapter binding ---'
sed -n '1,35p' src/shared/agent-hook-listener/listener-state.ts
rg -n -C 22 'class .*Legacy|admit\(|AGENT_STATUS_2A_CURRENT_PRODUCER_MODE|legacyStatusAdapter' src/shared/agent-status-legacy-adapter.ts src/shared/agent-hook-listener src/shared | head -n 260
printf '%s\n' '--- IPC mapper ---'
rg -n -C 30 'function toAgentStatusIpcPayload|const toAgentStatusIpcPayload|toAgentStatusIpcPayload\s*=' src/main/agent-hooks/server src/main src/shared | head -n 220
printf '%s\n' '--- hydration boundary ---'
sed -n '1,145p' src/main/agent-hooks/server/server-hydration.ts

Repository: stablyai/orca

Length of output: 42684


🏁 Script executed:

#!/bin/bash
set -eu
printf '%s\n' '--- legacy adapter storage and admission ---'
sed -n '90,245p' src/shared/agent-status-legacy-adapter.ts
printf '%s\n' '--- persisted entry sanitizer ---'
rg -n -C 18 'function sanitizeHydratedEntry|const sanitizeHydratedEntry|sanitizeHydratedEntry\s*=' src/main/agent-hooks/server/server-persistence-validation.ts

Repository: stablyai/orca

Length of output: 5530


🏁 Script executed:

#!/bin/bash
set -eu
sed -n '34,135p' src/main/agent-hooks/server/server-persistence-validation.ts

Repository: stablyai/orca

Length of output: 4195


Do not retain authorityRestartId in refreshed rows or persisted status data.

The destructure omits authorityRestartId, so ...payload copies it into refreshed. The legacy adapter stores that object, and persistence retains the field in the serialized entry. The IPC snapshot mapper strips the field, but enriched-status emission can still forward the stale value. Destructure authorityRestartId before spreading payload.

Proposed fix
       restoredUnconfirmed: _restoredUnconfirmed,
       isReplay: _isReplay,
+      authorityRestartId: _authorityRestartId,
       ...payload
📝 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.

Suggested change
}
const {
receivedAt: _receivedAt,
evidenceObservedAt: _evidenceObservedAt,
stateStartedAt,
observation: _observation,
restoredUnconfirmed: _restoredUnconfirmed,
isReplay: _isReplay,
...payload
}
const {
receivedAt: _receivedAt,
evidenceObservedAt: _evidenceObservedAt,
stateStartedAt,
observation: _observation,
restoredUnconfirmed: _restoredUnconfirmed,
isReplay: _isReplay,
authorityRestartId: _authorityRestartId,
...payload

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ℹ️ No new issues — the only delta since the prior review is a behavior-preserving refactor.

Reviewed changes

  • Extracted remote-envelope normalization: normalizeRemoteEnvelopeFields moved from inline logic in server-ingest-remote.ts into new server-remote-envelope-normalization.ts, with byte-identical field handling.
  • Moved status-application helpers: refreshTerminalStatusEvidence and writeLegacyStatusRow moved from AgentHookServerStatusUpdate up to AgentHookServerStatusApplication; writeLegacyStatusRow relaxed from private to protected. No subclass between the two defines either name, so the move is a pure relocation.
  • Comment trimming: two multi-line Why: comments on the legacy compact-boundary and trigger-label paths were condensed to one line each.

Verified with pnpm tc:node (pass) and the focused OMP suites (server-omp-retired-ingress, server-omp-retirement-ack, server-retired-pane-new-turn: 39 passed).

Pullfrog  | Fix it ➔ | View workflow run | Using DeepSeek Flash (free via Pullfrog for OSS) | 𝕏

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ℹ️ No new issues — the only change since the last review is a one-line manifest path correction.

Reviewed changes

  • Updated the legacy ingress manifest: AGENT_STATUS_LEGACY_INGRESS_MANIFEST's main-status-update entry now points sourcePath at server-status-application.ts, the file the admitLegacyAgentStatus call moved to in 602b49881c. agent-status-legacy-ingress-ratchet.test.ts compares declared and actual call sites in both directions, so the correction is required, not cosmetic (verified locally: 14 passed).

Pullfrog  | Fix it ➔ | View workflow run | Using DeepSeek Flash (free via Pullfrog for OSS) | 𝕏

@nwparker
nwparker merged commit e8a7be4 into main Sep 19, 2026
35 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: Re-attached Pi pane permanently suppresses working/done status in main and renderer retired-pane tombstones

1 participant