Skip to content

fix(paseo-omp): correlate terminal events by request - #81

Merged
omercnet merged 1 commit into
mainfrom
investigate-agent-end-omp-session
Sep 17, 2026
Merged

omercnet merged 1 commit into
mainfrom
investigate-agent-end-omp-session

Conversation

@omercnet

@omercnet omercnet commented Sep 17, 2026

Copy link
Copy Markdown
Owner

Summary

  • accept a later-turn agent_end only when its RPC requestId matches the active prompt
  • reject keyed stale terminals before startup buffering or any turn-state mutation
  • scope streamed terminal evidence to each native run across nonterminal continuations
  • preserve legacy fail-closed behavior for unkeyed OMP runtimes
  • retain request identity through degraded terminal envelopes and document the version-gated contract

Companion OMP protocol change: can1357/oh-my-pi#12331

Evidence

Today’s daemon logs contained 12 OMP terminal ownership could not be confirmed failures across six persisted sessions. Each was followed by a native OMP SIGTERM at the two-second ownership deadline. Request identity on the terminal itself removes that ambiguity without trusting branch text, idle state, or event timing.

Verification

  • npm test -- tests/provider.test.ts tests/omp-rpc.test.ts: 359 passed
  • npm run typecheck: passed
  • npm run check: passed
  • npm test: 684 passed, 3 skipped
  • CI: all 16 jobs passed, including Windows, macOS, Ubuntu, WSL, and the real OMP version matrix
  • Astra final review: approved, no remaining blockers

Release Please owns CHANGELOG.md and package versions, so neither is changed.

@omercnet
omercnet force-pushed the investigate-agent-end-omp-session branch from 685dd02 to 302088d Compare September 17, 2026 11:37
@omercnet omercnet changed the title fix(paseo-omp): correlate deferred terminal completion fix(paseo-omp): correlate terminal events by request Sep 17, 2026
@omercnet
omercnet force-pushed the investigate-agent-end-omp-session branch from 302088d to eb8ff1d Compare September 17, 2026 11:50
@omercnet
omercnet marked this pull request as ready for review September 17, 2026 11:53
Copilot AI lite review requested due to automatic review settings September 17, 2026 11:53

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@omercnet
omercnet force-pushed the investigate-agent-end-omp-session branch from eb8ff1d to 5ced640 Compare September 17, 2026 13:08
@omercnet
omercnet merged commit 41f756e into main Sep 17, 2026
16 checks passed
@omercnet
omercnet deleted the investigate-agent-end-omp-session branch September 17, 2026 13:16
@Trevor-Mengel

Copy link
Copy Markdown
Contributor

Tested this branch against the fault it targets. Short version: it fixes the second-prompt failure when paired with an OMP build that carries can1357/oh-my-pi#12331, and it does not change the outcome on any published OMP (18.2.1 and 18.2.4 both still fail).

What we see in production

Paseo 0.8.0, paseo-omp at b265ad31, OMP 18.2.1, macOS arm64. On every OMP runtime the first prompt completes, the second ends with OMP terminal ownership could not be confirmed about 2 s after agent_end, the runtime's process group is terminated, and the third prompt succeeds on a rebound runtime. It is not specific to any command or model; it reproduces with a one-word prompt.

Method

No daemon. I reused the shape of tests/provider.real.e2e.test.ts (real omp child through createOmpProvider + PluginAgentClientRegistry, hermetic local OpenAI-compatible server) but ran three sequential session.run() calls on one session, selecting the binary with OMP_COMMAND. The existing real-e2e test runs a single prompt per session, which is why it does not see this.

  • Plugin trees: b265ad31 (byte-identical server/ to what we run) and this PR's head 5ced6406.
  • OMP: 18.2.1 (global install), 18.2.4 (npm, scratch-local), and the #12331 head 1de79492 run from source with the published 18.2.4 pi_natives darwin-arm64 addon (the PR touches no native code).

Result (prompt 1 / 2 / 3)

plugin ╲ OMP 18.2.1 18.2.4 #12331 1de79492
b265ad31 ok / ownership error at ~2.04 s / – ok / ownership error / – ok / ownership error / –
this PR 5ced6406 ok / ownership error at ~2.04 s / – ok / ownership error / – ok / ok / ok (39 ms, 15 ms)

(Prompt 3 is "–" where prompt 2 failed because the harness uses persistSession: false, so there is nothing to rebind to. Under the daemon the third prompt succeeds.)

Focused suites on this branch: tests/provider.test.ts + tests/omp-rpc.test.ts 359 passed; npm run typecheck clean.

Why published OMP still fails

On an ordinary later turn the only sources of terminalOwnershipEvidence are a positive prompt_result/acknowledgement (agentInvoked: true, which OMP emits only for consumed builtins — rpc-mode.ts sends prompt_result solely on the agentInvoked:false path) or, with this PR, requestId on agent_end. Since #69, publishCorrelatedUser marks evidence only when !turn.terminalOwnershipRequired, so for an unkeyed runtime nothing can ever satisfy the check and every second prompt hits the AGENT_END_STATE_TIMEOUT_MS deadline. That matches the PR text ("legacy fail-closed"), I just want to make the practical consequence explicit: until an OMP release includes #12331, every published OMP version loses every second prompt and has its process group killed.

Two suggestions, take or leave:

  1. A multi-prompt case in the real-e2e job would have caught this and will guard the keyed path once CI runs an OMP that has #12331.
  2. For the interim, an opt-in legacy mode (default off, so the safety boundary of fix(paseo-omp): fail closed on unowned later-turn terminal events #69 is unchanged) that accepts the pre-fix(paseo-omp): fail closed on unowned later-turn terminal events #69 correlated-user evidence on runtimes that do not key agent_end. I'm happy to send that as a separate small PR if it is welcome; I won't duplicate this one.

Happy to share the three-prompt test file if useful.

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.

3 participants