refactor(driver): unify the pi/omp session scaffolding and the exit-label map - #510
Merged
schickling-assistant merged 4 commits intoSep 7, 2026
Conversation
… terminal `provider_session` spawns the provider with `stderr(Stdio::inherit())`, so this `eprintln!` painted "recording session end failed" straight into the interactive harness's own terminal — inside the agent's UI, where a human is reading a session, not a log. pi's twin already used `tracing::warn!`. It lands first because the pi/omp unify that follows deletes this line: with the two copies merged there is exactly one sink, and this is the one they should share. No test asserts the wrapper's stderr, so this is review-pinned only. agent-identity: dev3.direct.omp.43sz6ujq agent-persona: generalist agent-supervisor: unavailable agent-tool: OMP agent-tool-version: 18.1.7 agent-runtime: OMP 18.1.7 tooling-profile: dotfiles@39a19af
pi and omp are the same harness family launched the same way: an extension loaded into the interactive process that reaches st2 by spawning `st2 driver <harness>-channel`. `omp_session.rs` carried a forked copy of the whole launch body, and the two copies had already drifted in doc rationale and in style without any behaviour differing. The fork now lives where the family's channel side already puts it: a `HarnessKind` descriptor plus a shared `run_for`, mirroring `pi_channel`'s `ChannelKind` — the same fork solved the same way twice rather than two different ways. What stays per-harness is what genuinely is: each module's doc, its extension asset, its channel variable NAMES (two disjoint sets so an omp seat can never adopt a stray pi configuration — `pi_channel` also reads them by path), omp's measured-build fixtures, and omp's launch-time version gate. That gate rides on the descriptor as `verify_version` and runs exactly where omp ran it: after the empty-argv check and before `harness_state::claim`, so an unadmitted minor still fails without claiming ownership of the seat. Where the two copies carried different rationales for identical code, the merged item carries both halves: the offline defaults keep pi's "a supervised seat must not update-check at boot" and omp's open DQ-OMP-5 note. No behaviour change: every string, ordering and write is preserved — the exit label still spells `(None, None)` as "exited" here, and the session-end warning already went to the tracing log on both sides as of the preceding commit. agent-identity: dev3.direct.omp.43sz6ujq agent-persona: generalist agent-supervisor: unavailable agent-tool: OMP agent-tool-version: 18.1.7 agent-runtime: OMP 18.1.7 tooling-profile: dotfiles@39a19af
Five copies of `(status, signal) -> label` existed; the pi/omp unify folded two of them. The remaining copies disagreed on exactly one arm: pi/omp spelled `(None, None)` as "exited", everyone else as "exit unknown". `provider_session::describe_exit` becomes the one map, and codex's `describe_tui_exit` keeps only what is genuinely its own — flattening a status that may never have been observable at all. Behaviour change, deliberate: the pi/omp record's `exit` field spells `(None, None)` as "exit unknown" instead of "exited". "exited" claimed an ordinary end for a status that answers neither question, which is the one thing that arm cannot know. Two facts de-risk it. The arm is unreachable for a child this process reaped: `Child::wait`/`try_wait` call `waitpid` with neither `WUNTRACED` nor `WCONTINUED`, so every status they return satisfies `WIFEXITED` or `WIFSIGNALED`; only `ExitStatus::from_raw(0x7f)` constructs it, in a test. And no reader parses the label back — `agents::ObservedJson` and `main.rs` pass it to display, `harness_state` only asks `exit.is_some()`. The pin the report claimed existed did not: `provider_session.rs`'s assertion is fed by `SessionObserver::launch_error()`'s hardcoded literal and never reaches this map. `the_exit_label_map_covers_every_arm` is that missing pin — the whole table, `(None, None)` included. agent-identity: dev3.direct.omp.43sz6ujq agent-persona: generalist agent-supervisor: unavailable agent-tool: OMP agent-tool-version: 18.1.7 agent-runtime: OMP 18.1.7 tooling-profile: dotfiles@39a19af
Five class-(a) duplications, each verified identical before folding:
- `channel_content` and `write_json` (claude_mcp, pi_channel) were
byte-identical. Both are stdio-native-channel framing decisions st2 owns
rather than each channel, so they move to `native_channel`. The NDJSON
framing gains the pin it never had: one newline-terminated line per value,
including a body that carries its own newline.
- `ProviderAuthEdge` had two byte-identical definitions; one moves into
`driver_diagnostic`, which already owns `Driver`/`Stage`/`Reason`/`Source`/
`Support`.
- `publish_provider_auth` was identical modulo a hardcoded `Driver`, so it
joins the enum and takes the driver as an argument, as pi_channel's copy
already did.
- opencode's `completed` folds onto `completed_provider("opencode", …)`:
"{provider} provider exited with {exit}" reproduces its message byte for
byte.
Where the two copies carried different rationales, the merged item carries
both: `publish_provider_auth`'s `None`/`Support::Unknown` now states both why a
Claude hook payload has no version to publish AND why a pi-family channel has
none (the wrapper owns the gate, OMP-R05).
`provider_auth_edge` stays forked in both modules on purpose: the two decode
disjoint inputs — a hook event name plus JSON payload versus a typed
`TurnResult` — and share only the output enum this commit unifies.
agent-identity: dev3.direct.omp.43sz6ujq
agent-persona: generalist
agent-supervisor: unavailable
agent-tool: OMP
agent-tool-version: 18.1.7
agent-runtime: OMP 18.1.7
tooling-profile: dotfiles@39a19af
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
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.
Tier-1 item
T1-DUPfrom #504, held back from #499–#502 because the dedupe carries deliberatebehaviour changes and folding those into a "no semantic change" PR is how they ship unreviewed.
Each behaviour change is its own commit, named below with its pin.
Four commits, in this order:
fix(omp)— the session-end diagnostic moves off the agent's terminal (behaviour change).refactor(driver)— the pi/omp launch body becomes oneHarnessKindfork (no behaviour change).refactor(driver)— one exit-label map (behaviour change: one wire-visible label).refactor(driver)— the byte-identical adapter copies fold (no behaviour change).The fix leads deliberately. omp's
eprintln!lives inside the function commit 2 deletes, so had itlanded last the unify itself would have silently carried it — the exact failure mode this split
exists to prevent.
Deliberate behaviour changes
(a) The
(None, None)exit label flips from"exited"to"exit unknown"for pi and ompWire-visible: it is the
exitfield of theharness-staterecord. Five copies of the(status, signal) -> labelmap existed and disagreed on exactly this one arm — pi/omp said"exited",provider_session,opencode_sessionand codex said"exit unknown". The unifiedlabel is
"exit unknown":"exited"claims an ordinary end for a status that answers neitherquestion, which is the one thing that arm cannot know.
Pin:
provider_session::tests::the_exit_label_map_covers_every_arm— a(status, signal) -> labeltable over every arm,(None, None)included. This pin did not existbefore. Note the correction to the report in #504: the claim that the unification is "pinned by
provider_session.rs:337" is false — that assertion is fed bySessionObserver::launch_error()'s hardcoded"exit unknown"literal and never calls the map.Reachability, verified in this tree, not assumed. The arm is unreachable for a child this
process reaped.
Child::wait/try_waitcallwaitpidwith neitherWUNTRACEDnorWCONTINUED, so every status they return satisfiesWIFEXITEDorWIFSIGNALED, andcode()orsignal()answers. The only construction isExitStatus::from_raw(0x7f)— the stopped wait status— and
grep -rn "from_raw" src/ crates/finds exactly fourExitStatus::from_rawcall sites inthe whole tree (two in
provider_session, two inpi_family_session), every one of them inside a#[cfg(test)] mod tests. Every otherfrom_raw*hit isFromRawFd.Every
ExitStatusreaching the pi/omp label comes fromProviderOutcome, whose two variants arebuilt only from
child.try_wait()andchild.wait()inprovider_session::run_provider_observedand
stop_provider_group.No reader parses the label back. Every consumer of the
exitfield either displays it or asksis_some():exitagents::ObservedJson(src/agents.rs:179)observed.exit.as_deref()straight into JSON outputsrc/main.rs:1131.exit.as_deref().map(str::to_owned)for displayharness_statecoalescing (:420,:433)exit.is_some()and whole-field equalityharness_stateclaim eligibility (:891)record.exit.is_some()— "is this a real terminal record"No
matchon the string, no prefix test, no split.grep -rn '"exited"'acrosssrc/,tests/,crates/,hooks/andcomponents/finds no consumer of the harness-state literal: every other"exited"in the tree is the unrelated PTY task lifecycle word (runtime.state), which this diffdoes not touch.
(b) omp's session-end failure moves off the agent's stderr into the tracing log
provider_sessionspawns the provider withstderr(Stdio::inherit()), so omp'seprintln!("st2 omp driver: recording session end failed: …")painted into the interactiveharness's own terminal — inside the agent's UI, where a human is reading a session. pi's twin
already used
tracing::warn!; the merged item keeps the log.Pin: none, and that is stated rather than papered over. No test asserts the wrapper's stderr,
and adding a process-level stderr-capture test for one warning line would be a heavier fixture than
the change deserves. It is review-pinned only, and it is its own commit precisely so a reviewer sees
it as a change of destination rather than as three lines inside a 700-line refactor.
Doc-rationale merges
Several of these pairs carried different rationales for identical code. The rationale is the part
that was genuinely not duplicated, so the merged item carries both halves:
OFFLINE_DEFAULTS— pi's ("a managed agent that update-checks or self-updates at boot makesits own launch latency depend on the network, and lets a release change a running fleet") plus
omp's open question ("whether they suppress the update banner in interactive boots is still open,
DQ-OMP-5; shipping them is harmless either way"). Both survive on the shared const.
publish_provider_auth'sNone/Support::Unknown— Claude's ("a hook payload carries noClaude version — session id, transcript path, cwd, prompt id, permission mode, agent identity and
effort, and nothing else (2.1.259) — and st2 gates no Claude version") plus the pi family's ("the
WRAPPER, not the channel, owns the version gate and refuses the launch on an unadmitted MINOR,
OMP-R05, so a running channel has no version fact of its own to publish"). Both are now stated on
the one function, because they answer the same question for different reasons.
publish_provider_auth's "fresh publisher per edge" — Claude's ("each hook invocation is itsown process, so the stage set starts empty and the on-disk fallback is what lets a later
Stopclear a rejection an earlier
StopFailurewrote") plus pi_channel's ("a channel that restartedmid-session inherits the predecessor's record rather than silently starting clean").
with_channel_extension— pi's doc is a strict superset of omp's: it carries the$ST_HOOKStoken failure mode and omp's "a rendered machine-local path would pin one host's layout into a
catalog". Both clauses are on the merged item; nothing was dropped.
record_session_end— pi's doc adds why theWriteris constructed at the terminal edge (itre-reads what the channel last wrote and continues its transition counter); omp's is contained in
it. Kept in full, generalized from "pi" to "the harness channel".
describe_exit— gains the reachability rationale above, so the next reader does not have tore-derive why the arm says "unknown".
What stays per-harness, and why
pi_channel.rsreads both modules'CHANNEL_*bypath (
PI_KIND/OMP_KINDinpi_channel), and the whole point of two sets is that an omp seatcan never adopt a stray pi configuration.
HarnessKindreferences them; it does not own them. Newpin:
pi_family_session::tests::the_two_harnesses_export_disjoint_channel_variable_names.verify_version: Option<fn(&str) -> Result<()>>and is called exactly where omp called it: after the empty-argv check and before
harness_state::claim, so an unadmitted minor still fails without claiming ownership of the seat.pi's slot is
Nonerather than a function that always succeeds — a gate that cannot refuse is nota gate. New pin:
omp_session::tests::the_version_gate_is_wired_into_the_shared_launch_fork.provider_auth_edgestays forked. The two decode disjoint inputs — a hook event name plus aJSON payload versus a typed
TurnResult— and shared only the output enum, which this PR unifies.Adapters folded, each verified identical first
provider_session::describe_exit(see (a))with_channel_extensionoffline_defaults+ its constchannel_envrecord_session_endchannel_contentdiffclean) →native_channelwrite_jsondiffclean) →native_channelpublish_provider_authDriver→driver_diagnostic, driver as an argumentenum ProviderAuthEdgediffclean) →driver_diagnostic, which already ownsDriver/Stage/Reason/Source/Supportcompletedcompleted_provider("opencode", …):"{provider} provider exited with {exit}"reproduces"opencode provider exited with {exit}"byte for bytechannel_contentandwrite_jsonland in a new crate-internalnative_channelmodule rather thanin either channel: what a delivered inbox message looks like to the model, and how a frame is
terminated on the wire, are st2's decisions rather than each channel's. The NDJSON framing also
gains the pin it never had — one newline-terminated line per value, including a body carrying its
own newline.
Not done, with the evidence
Every item below was re-checked against this tree, not taken on the report's word.
compaction_triggerunification — skipped.claude_session::compaction_trigger(
src/claude_session.rs:355-357) decodes{manual, auto}and everything else toUnknown;pi_channel(:564-568) decodes five words. Unifying would widen Claude's decode vocabulary to{manual, auto, threshold, overflow, idle}, and the narrow set is a documented decision pinned byan_unrecognized_trigger_word_decodes_as_unknown_not_as_a_definite_one, which asserts Claudedecodes
"idle"asUnknown. That test would have to be deleted to land the "dedupe".--versionbanner capture — skipped.harness_version's module docstates the divergence is intentional: "omp gates the launch, opencode degrades to no native
delivery, and codex refuses semantic-version reasoning outright." Only the parsing is shared, and
it already is. Extraction would also change one of opencode's error strings.
completed_tui— skipped. Its message is"controlled Codex TUI exited with {status}"(src/codex_app_server.rs:2897), whichcompleted_providercannot reproduce: an operator-visible string change for zero structural gain.stop_provider_group— skipped, genuinely non-extractable.provider_session's takes anobserver and writes the terminal record on two paths opencode's has no equivalent for (the
graceful reap and the pre-SIGKILL cover); opencode's takes only the child, because its observation
ownership lives in a different type.
anyhow::ensure!(exit.success(), "{label} provider exited with {exit}")— leftinline rather than routed through
completed_provider. It is byte-identical and would foldcleanly, but it is not on the sheet, and this PR's job is the enumerated set.
Verification
CARGO_TARGET_DIR=/tmp/st2-lane-cargo CARGO_BUILD_JOBS=8 nix develop --command cargo check --all-targets— clean, and no warning in any file this diff touches. (The tree's pre-existing warnings in
resource_profile_supervisor,ding/mod.rs,tests/resync.rsandcodex_app_server'sprivate_interfacesare unchanged.)cargo test --lib -- pi_session:: omp_session:: pi_family_session:: provider_session:: opencode_session:: codex_app_server:: pi_channel:: claude_session:: claude_mcp:: native_channel:: driver_diagnostic::— 168 passed, 0 failed (run three times).cargo test --test driver_expansion --test harness_state_teardown --test up_once_exit—13 + 3 + 4 passed, 0 failed.
harness_state_teardownis the one that exercises a real wrapper'sterminal record end to end (
exit="signal 9","stopped","signal 15").git diff origin/main --numstatoveromp_session.rs,pi_session.rs,pi_channel.rs,claude_mcp.rs,claude_session.rs,opencode_session.rs,codex_app_server.rsis+80 / −670 (net −590). The whole-diff net is +39, because the shared modules carry the
merged body, both halves of every merged rationale, and five new pins (~100 lines of tests that
did not exist). Stated plainly: this removes a duplicated body, not line count.
Independent review
A reviewer with a clean context was asked to falsify the exclusivity claim above. Verdict: the two
stated behaviour changes are the only ones — no third change, no bug introduced by the fold. It
re-derived the unify by normalizing both original launch bodies and diffing them (operationally
identical apart from the harness word, omp's version gate, doc wording, the stderr sink and a type
alias), confirmed the version gate still runs before
harness_state::claim, confirmed all fiveexit-label copies are folded with every non-
(None, None)arm spelled identically, re-ran thereachability greps, and checked each adapter fold against BOTH originals.
Two corrections it made to this description, neither changing the verdict:
exit-field readers isagents.rs:179,main.rs:1131, andharness_state.rs:420(is_some),:433(whole-fieldequality),
:474(pass-through),:614(is_none, combined withreason == "superseded"),:646(pass-through),:891(is_some). None parses, prefixes or splits the string, so theconclusion holds.
pi_channelalready has private consts namedPI_KIND/OMP_KINDof typeChannelKind, and the newHarnessKindconsts reuse those identifiers indifferent modules. Legal and non-colliding, but two same-named consts of different types now live
in the family.
Posted on behalf of @schickling
agent_identitysessionagent_personaagent_supervisoragent_toolagent_tool_versionagent_runtimetooling_profile