Repository navigation
fix(omp): find transcripts in the store the pane's omp process moved - #1898
Conversation
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 48 minutes. View limit details
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 |
Code review standing in for CodeRabbitCodeRabbit hit its review limit on this PR, so no CodeRabbit review ran. This PR changes which session paths the conversation route accepts. The reviews below were run on this change, and each finding's resolution is listed. omo-native-code-reviewer, second pass on the final diff (73dbbcd; same content as cd75a0d0c plus one test fix)Verdict: APPROVE. No CRITICAL, HIGH or MEDIUM findings. It ran It confirmed the fixes made after the first pass and the Codex review:
It found no symlink escape and no secret leaving the environment allowlist. GJC callers behave as before. LOW notes and how each was resolved:
omo-native-code-reviewer, first pass (earlier version): APPROVEAll of its points were resolved:
Codex (Sol 6.1 Fast, xhigh)
Reports are kept locally in the lane's worktree: |
|
Merge pass gate review of 73dbbcd: two blockers. The moved-store behaviour itself works, and 51 targeted tests pass. 1. When the only omp process can't be read, the decoy check is skipped (
2. The native Linux environment read has no bound (
Holds
The merge pass re-checks the fix commit (changed range only). |
The bridge only knew ~/.omp/agent and --profile stores, read from its own HOME, so an omp pane started with --session-dir, PI_CODING_AGENT_SESSION_DIR, PI_CODING_AGENT_DIR, PI_CONFIG_DIR or XDG data/state roots showed no chat. ompStoreLayout now derives the store from the pane's omp process argv and routing environment the way omp's own dirs.ts does (verified against the real omp 18.8.7 by scripts/omp-store-layout-probe.ts, 47 cases, fixture server/fixtures/omp-18.8.7-store-layouts.json). herdr's path and the held fallback are both held to that store: links out of it, another folder's session in a session directory of its own, unsupported session versions, unreadable environments and conflicting stores show no chat. Fixes #1613
… refuse a lone unread omp Merge pass gate review of #1898: - A pane whose only omp process could not be read got a null route, which fell back to the bridge's own default store for herdr's path. A live process whose argv and environment cannot be read now makes the route unproved whether or not another omp was read. - On native Linux with herdr's argv, the route read /proc/<pid>/environ synchronously in the bridge. Every Linux read now goes through the bounded helper (timeout, size cap, refuses a process in D state), which fails closed to unproved. Tests: every Linux read now goes through the real helper, so the chat tests give it a deadline no busy PC reaches, and the routing-key test uses #1862's started shell target, as omp-process.test.ts does.
73dbbcd to
9d8b65f
Compare
|
Both blockers from the gate review are fixed in 9d8b65f. The branch is also rebased onto main 3346b03, which brings in #1862, so this push is a force push. 1. When the only omp process can't be read, the decoy check was skipped. In
2. The native Linux environment read had no bound. On Linux every read of an omp process now goes through the bounded helper Because every Linux read now uses the real helper, the chat tests give it a 30 s deadline, as #1862 did for Verification on 9d8b65f
Outside this PR: |
|
I reran the real-omp QA on 9d8b65f, the same commit as before and after the run, and all 22 checks passed. The setup was a real omp 18.8.7 in a pane of
On herdr's first answer, the helper couldn't read the fresh omp, which was still in D state while paging in its binary. Its read was not cached, and the next poll read it, so the pane resolved to the flat store at |
|
Merge pass review-work report. Overall: PASSED.
Fixed in 9d8b65f
The read the re-check held is not from this PR. Holds from round 1
All required checks are green and no review thread is open. CodeRabbit did not review this PR (rate-limited or skipped). The reviews above stand in for it. |
What it does for the user
An omp pane whose sessions live outside
~/.omp/agentnow shows its chat. The bridge works outwhere the pane's own omp process writes its sessions and terminal breadcrumbs, from that process's
argv and environment, never from the bridge's own:
--session-dirandPI_CODING_AGENT_SESSION_DIR(a flat session directory of its own),PI_CODING_AGENT_DIR,PI_CONFIG_DIR, the process'sHOME,--profile,OMP_PROFILE,PI_PROFILE), and$XDG_DATA_HOME/omp,$XDG_STATE_HOME/omp).Both ways of finding the transcript use that layout: the path herdr names (the hook path), and the
file the omp process holds open or its terminal breadcrumb (the held fallback).
When the bridge can't prove where the store is, the pane shows no chat. It doesn't guess:
omp writes there, after following links. In a session directory of omp's own, where every
folder's sessions sit side by side, its header must also name the pane's live directory.
Otherwise the held fallback answers, and it tries the absolute
--cwdevery omp process startedwith only after the live directory. A default-store session no longer passes for a moved store's
session.
the breadcrumb. So is a subagent file whose parent session links out, which the old
gjcSessionFileaccepted.and 18.12.6 both write 3).
omp whose environment can't be read, a variable given twice, a relative directory, an empty
--session-dir, or a profile name omp refuses.PI_CODING_AGENT_DIRset to the default agent directory keeps the XDG roots, as native omp does. Theold code skipped them. A value that names a profile's agent directory and was handed down by that
profile is ignored, as omp ignores it.
Why
Fixes #1613. The bridge only knew
~/.omp/agentand--profilestores, read from its ownHOME,and treated
--session-diras "unknown". A pane whose launch moved the store showed no chat. Theearlier local branch's review found three more gaps, fixed here:
How the layout is derived
server/omp.tsompStoreLayoutfollows omp's ownpackages/utils/src/dirs.ts. I read it from theinstalled 18.8.7 binary and from the published 18.12.6 source.
scripts/omp-store-layout-probe.tsstarts the installed omp 18.8.7 in a temporary HOME for eachrouting case (no prompt is sent; provider traffic goes to a closed port). It records where omp
writes its breadcrumb and session, checks that the resolver gives the same roots and finds that
session, and writes
server/fixtures/omp-18.8.7-store-layouts.json. The probe exits 1 when anysupported case lacks that proof, when an unsupported one resolves, or when a secret reaches the
helper's answer.
The routing variables come from the process itself:
process in D state), which now passes on the routing keys and nothing else. The bridge never
reads
/proc/<pid>/environitself. A process the helper can't read, whether herdr supplied itsargv or not, makes the store unproved.
darwinProcess.environment, already used for Claude).omp reads the XDG roots on macOS only from 18.12, so an XDG omp root there counts as unproved.
never takes a custom session directory.
Settings that omp loads at runtime from a
.envfile aren't visible. Such a pane shows no chat:its files sit outside the store the bridge predicts. GJC's defaults are unchanged.
ompAgentDir(used by the composer's integration check) is untouched.Verification
PI_CODING_AGENT_SESSION_DIR,--session-dir,PI_CODING_AGENT_DIRand XDG roots, main'shook-store check and held lookup both returned nothing. The default case was found.
PI_CODING_AGENT_DIR, relative--session-dir, empty values) resolved to nothing, and no secret reached the helper's answer. The committed fixture is that run's output; an earlier run produced the same bytes.server/omp.test.tspasses 34/34 andserver/omp-process.test.ts7/7 on 9d8b65f. Inbun run check faston 9d8b65f, the unit suite passed 6423, failed 0 and skipped 13 across 456 files; it coversomp,omp-process,gjc-runtime,omp-incrementalandwindows-native.server/omp.contract.test.tspassed 2/2 underbun run check runagainst the isolated herdr session.PI_CODING_AGENT_SESSION_DIRset. Two servers (this branch and origin/main) were queried withcurl -i /api/pane/conversation. This branch found the moved session through the held file (on the second poll, once omp left the D state it sits in while paging in its binary) and through the hook path. With a decoy session of the same folder in the default store, it showed the moved session on 5/5 polls. origin/main showed scrollback on 5/5 polls for the held and hook cases, and the decoy on 5/5 polls.PI_CODING_AGENT_DIRnaming the default agent dir, the handed-down profile rule, an unreadable environment treated as the default, a relative session dir, macOS XDG, refusing the moved-store hook path, the depth-1-only header check, and the unread-owner check. One mutation, accepting the default store, stays green alone because the canonical containment inompRouteOwnsrefuses the decoy too. With both removed, the decoy test fails.bun run check fast: passes on 9d8b65f: workflow syntax, generated types and themes, typecheck, build, unit tests and actionlint.This changes which session paths the conversation route accepts, so the store-containment checks
need a real review.
Reviews
.envrouting values aren't seen.ompRouteOwnsrefuses herdr's path). The others are left as follow-ups. A stale herdr path in a managed store and a pane whose only omp can't be read both behave as on main.server/core/composer.tsstill finds the integration through the olderompAgentDir, which is outside this issue.server/omp.ts.git merge-treemerges the two cleanly.Merge pass gate review (fixed in 9d8b65f)
/proc/<pid>/environread.Details and tests are in the reply.