Repository navigation
fix(omp): show exit 0 for a command that finished normally - #26494
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (2)
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 2 remain after this review. 📝 WalkthroughWalkthroughThe OMP dialect now recognizes timeout, signal, async, and service details when normalizing tool results. If a completed foreground command has no reported exit code and meets the new conditions, normalization sets its exit code to 0. Otherwise, it leaves an omitted exit code undefined and retains any reported exit code. Tests cover these cases and update the successful-command recording expectation. Priority: ⬇️ Low Merge Risk: ⚪ Minimal · up to Successful foreground OMP commands now show exit 0, and failed commands keep the code OMP reported. A background command may rarely show exit 0 if neither async indicator is present. This is a disclosed limitation with low impact, so the change looks safe to merge. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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 |
|
OMP normally omits an exit code when a shell command succeeds, so Orca showed its output without Review found and fixed one PR-caused P2: OMP 18.4 can finish a service-start tool call while its named service is still running. Inferring zero from tool completion alone incorrectly claimed process exit. The final change requires foreground elapsed time and excludes service/background results; explicit reported codes remain authoritative. The regression uses the verified 18.4 producer shape, with a separate defensive service-with-timing case. A fresh narrow reviewer confirmed the fix at The foreground timing gate, asynchronous-result exclusion, and preservation of generic results follow those parts of the common pattern. Intended differences: Orca retains its command-echo recognizer and normalization of each current update, preserves output when execution metadata is absent without inventing an exit, and excludes service results explicitly. No new process registry or persisted state was added. Final-head validation: 13 focused tests across two files, queued node typecheck, and all 22 static checks passed. All 35 CI checks completed: 20 succeeded and 15 were skipped; skips are not test passes. Proportional final-diff readiness review completed without counsel. Live macOS QA used real OMP 17.0.5 with the existing login in place, an isolated hidden rig, and a plain folder workspace. Both commands ran once: The rig's saved and rendered native-chat settings and disabled hooks were verified before input. All recorded rig/provider/driver processes exited, and the own laptop child Remote QA required the authorized Out-of-scope observation only: the Windows session-history page showed “Preparing the WSL SQLite reader” and “0 of 45 sessions,” suggesting history outside the isolated home. No diagnosis or product change was made for it. Live OMP 18.4 named-service execution, Linux/SSH/mobile behavior, and arbitrary custom producers were not exercised. Generic mixed result shapes remain unverified, with no supported producer example established. Historical rows are not retrofitted. No merge or auto-merge was performed. |
1 participant
ELI5
In an OMP chat, a shell command that succeeded showed its output but no exit code, while a failed one showed
exit 3. Now a foreground command that finished normally showsexit 0too; starting a still-running service does not.What Changed
The problem. OMP reports an exit code only when a shell command fails. Orca's OMP chat (#26401) showed what OMP sent, so successful commands had no exit code on their row. Review of this patch also found that an OMP 18.4.0 named-service start could be marked
exit 0while the service was still running: its tool call had completed, but its process had not exited.What is different for you. A command that ran in the foreground and finished normally now shows
exit 0. Failed commands retain the code OMP reported, such asexit 3. Orca does not infer a code for a running, timed-out, signalled, background, or named-service result, or when foreground completion timing is missing.How.
src/main/acp/acp-dialects/omp-dialect.tskeeps OMP's reported code. Otherwise it infers zero only when the tool call iscompleted,details.wallTimeMsis present, and no timeout, signal, async, or service marker is present. Existing generic command results are left intact. Only an exit code OMP actually sent trims the "Command exited with code N" line; inferred zero cannot remove user output.Why
The foreground timing gate, timeout/signal checks, async exclusion, and generic-result short circuit follow the common pattern. OMP's foreground result builder supplies elapsed time after a finite run; service-start completion supplies service state instead. Reusing the existing normalizer keeps that distinction at the provider boundary without adding process tracking or stored background state.
Intended differences: Orca retains its existing command-echo recognizer and normalizes each current update before earlier input is restored. It still extracts output when execution metadata is absent, while leaving the exit code unknown. It also excludes every service result from inference, even if one includes timing, because a completed service-start call does not prove process exit. No temporary deviation is introduced.
Known limit
Zero is inferred from OMP's foreground completion contract, not from tool completion alone: wall time must be present and service/async results are excluded. OMP completion updates omit the original input, so the current result's timing and lifecycle markers provide the evidence. If OMP omits both a reported exit and foreground timing, the row retains output with no exit code; it does not guess success. The verified built-in foreground/service/background shapes support these rules; arbitrary custom producers or future contradictory metadata are unverified. Existing historical rows are not retrofitted.
Linked Issue
Follow-up to #26401 (OMP over ACP); no separate issue.
Visual Proof
Live on final head
7273a31657d, using real OMP 17.0.5 and the existing login on macOS in an isolated native-chat rig with a plain folder workspace. The command details were expanded through the rendered controls. The full screenshots show the command, output, and code; the own session journal independently records the same results.Successful foreground command:
printf 'ok\n'→ outputok,exit 0.Failed foreground command:
printf 'hi\n'; exit 3→ outputhi,exit 3.Testing
I manually tested these changes locally
Automated tests added/updated, or explained why not below
Final head
7273a31657d: 13 focused tests across two explicit files, queued node typecheck, and all 22 static checks passed. All 35 CI checks completed: 20 succeeded and 15 were skipped; skips are not test passes.omp-dialect.test.tscovers foreground elapsed time (including zero milliseconds), reported failures, timeout/signal/background/service exclusions, missing timing, generic results, and preservation of output. The running named-service regression uses the verified OMP 18.4 producer shape; a separate defensive case includes service metadata with timing.Live macOS OMP 17.0.5: both requested foreground commands ran once, and their expanded rows showed exit 0 and exit 3. Full screenshots are embedded above; actual session-journal records agree. Native-chat flags and disabled hooks were read back before input. The rig and its own laptop child worktree were removed after verification.
The full stable-file hash set was captured before launch, after activation, and after teardown with no read errors. Codex configuration (project trust rows stripped), hooks, and the other checked agent configurations were unchanged. OMP wrote its runtime databases, sidecars, and one session file while using its existing agent directory in place.
M4Air did not pass the shared RAM/load gate. Windows command rows were not exercised: OMP chat startup hit a separate pre-existing session-store failure, assigned to a separate investigation. The stopped Windows profile/database is preserved; its inactive QA checkout is also retained because Git still reports a lockfile modification despite verified identical file/index/commit bytes. The official removal command refused that modification; no force removal was used.
Live OMP 18.4 named-service execution, Linux/SSH/mobile behavior, and arbitrary custom producer metadata remain unverified. Existing historical rows are not retrofitted.