fix(hosts): rank pi session discovery by last write - #84
Merged
Merged
Conversation
Pi names a session file once, at creation, so filename order is creation order. A session resumed after days of idling lost to any session created since in the same folder, which is the "random message" a pane running an older pi session opens. Rank candidates by modification time instead, with filename order as the tie-break and for files whose metadata cannot be read. OMP discovery delegates to the same function, so it follows. refs plannotator/herdr-annotate#37
Merged
1 task
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.
Fixes the half of plannotator/herdr-annotate#37 that lives in this tree, and writes up the rest.
What changed
pi::find_transcriptranked candidate session files by filename. Pi names a sessionfile once, at creation (
<ISO timestamp>_<uuid>.jsonl), so filename order is creationorder, not use order. A pi session started days ago and resumed today therefore lost to
any session created since in the same folder — which is exactly the "opens very random
messages" the reporter sees when they annotate from a pane running an older session.
newest_with_messagesnow ranks by file modification time, newest first, and falls backto filename order on ties and for files whose metadata cannot be read (so a directory
whose files all share an mtime behaves as before). This is the rule pi itself uses:
findMostRecentSessionincore/session-manager.ts, behindpi --continue, sorts thebucket by
mtimeMs.omp::find_transcriptdelegates topi::find_transcript, so OMP follows; a test pinsthat. (OMP fallback discovery is unreachable in the app today —
last/fallback.rsbailsfor
Host::Omp— but the shared function is the same one.)Two existing fixture tests stage the fixtures into a temp dir with explicit mtimes now.
A checkout leaves every fixture the same age, in whatever order git wrote them, which
says nothing about which session was last used; relying on that would have made those
tests depend on checkout order.
Tests:
a_session_written_to_more_recently_wins_over_one_with_a_newer_name(hosts/pi)and
omp_ranks_candidates_by_last_write_like_pi(hosts/omp). Both fail onmain.cargo fmt --all --check,cargo clippy --workspace --all-targets -- -D warnings, andcargo test --workspaceare clean (22 test binaries, 0 failures).Scope
This fixes the fallback path — the one taken when Herdr reports no session for the
pane. It does not fix the two cases below, which are not resolvable in this repo.
Investigation
1.
/treenavigation is not observable from outside the pi processVerified in pi's source.
/treemoves the leaf and writes nothing:SessionManager.branch(id)isthis.leafId = branchFromIdand nothing else(
core/session-manager.ts:1436-1441);resetLeaf()isthis.leafId = null(
:1448-1450). No I/O in either.AgentSession.navigateTree(core/agent-session.ts:3282-3474) callsbranch()/resetLeaf()on the no-summary path (:3423-3437). It appends an entry only when theuser picks "Summarize" (
branchWithSummary,session-manager.ts:1457-1482) or pressesShift+L to label (
interactive-mode.ts:5487-5490). The TUI never passeslabelitself (
interactive-mode.ts:5455-5458)._rewriteFilecall sites are migration,zero-length repair, and
/forkonly:session-manager.ts:954,1004,1578).settings, auth, crash logs, trust, and user-invoked exports;
Settings(
core/settings-manager.ts:110-163) has no leaf or current-session field.--mode rpcis stdin/stdout JSON-lines (modes/rpc/rpc-mode.ts:1-13)and
RpcSessionState(modes/rpc/rpc-types.ts:96-109) carriessessionFile,sessionId,sessionName,messageCount— noleafId. The unix-socket coordinatoris
PI_EXPERIMENTAL=1-gated, reached through a different entry file, and excluded fromthe npm tarball (
packages/coding-agent/package.json:29-33).On load, pi rebuilds the leaf as the last non-header line in file order
(
_buildIndex,session-manager.ts:1014-1032) — the same rulepi::active_branchuseshere. So an outside reader is correct up to the moment the user navigates, and wrong
until the next entry is appended. If pi exits after navigating with no further message,
the navigation is lost even to pi.
The leaf is observable in-process, though.
session_treecarries it:{ type, newLeafId, oldLeafId, summaryEntry?, fromExtension? }(
core/extensions/types.ts:664-670, emitted atagent-session.ts:3463-3470) — the onlyevent that does. Extensions can also read
ctx.sessionManager.getLeafId()/getBranch()(read-only surface at
session-manager.ts:212-228).Minimal change, and where it belongs. Two options, both outside this repo:
session; add a
session_treesubscription that re-reports with the new leaf.PaneReportAgentSessionParams(src/api/schema/panes.rs:383-395) has nodeny_unknown_fieldsand the published schema does not setadditionalProperties: false, so anagent_session_leaf: Option<String>is additiveand old clients are unaffected. It would surface through
AgentSessionInfo(
src/api/schema/agents.rs:225-231) next tokind/value, and we would pass it as aPLANNOTATOR_TUI_SESSION_LEAFenv var and startactive_branchfrom that id insteadof the last line. Roughly: one event handler in the asset, one optional field through
schema →
agent_resume→TerminalState, one optional env var and one parameter here.backnotprop/plannotator,apps/pi-extension).It already resolves the live branch correctly —
getLastAssistantMessageSnapshotcalls
ctx.sessionManager.getBranch()(assistant-message.ts:70) — which is why/plannotator-lastinside pi is right today even after/tree. It could write theleaf to a sidecar next to the session file for plannotator-tui to read, but that is a
new private contract; option 1 is cleaner.
Nothing needs to change in pi itself.
2. The exact-session path, end to end, and every way it still goes wrong
Chain (verified): pi loads
~/.pi/agent/extensions/herdr-agent-state.ts(v8) → onsession_start(TUI mode only) and on everyagent_startit callsupdateSessionRefandsends
pane.report_agent_sessionwithagent_session_path(preferred) oragent_session_id→ Herdr validates and stores it on the pane ashook_authority .session_ref/persisted_agent_session→herdr agent get <pane>printsresult.agent.agent_session = {source, agent, kind, value}→ our launcher'sagent_identity(herdr/launch.rs:84) reads it andargvemitsPLANNOTATOR_TUI_SESSION=<path>orPLANNOTATOR_TUI_SESSION_ID=<id>(
herdr/launch.rs:289-297) →last/locate.rs:28expands~andreaders::explicitreads the file (path), or
exact::resolvefinds it by id in the cwd bucket(
last/exact.rs:45-50).Ways this still yields the wrong file, and how to tell from
herdr agent get <pane>:agent getsymptomHERDR_PANE_ID/HERDR_SOCKET_PATHmissing (ssh, tmux, docker exec)agent_sessionabsent (skip_serializing_if,agents.rs:206) — we then fall back to folder discovery, and the UI says "no session id from Herdr, showing the newest transcript for this folder"ctx.mode !== "tui"early return, asset 228-230;rootSessionnever set soagent_startalso bailsagent_sessionabsentvaluepersists/newreport rejectedsession_start_source ∈ {new, resume, fork}for pi (terminal/state.rs:1331);agent_startre-reports send no source and are dropped (state.rs:1548-1560)valueis the pre-/newpath. This is what Herdr #1189 / #943 addressedpi -r/pi -cat process startsession_startwithreason: "startup", not"resume"(agent-session.ts:411;"resume"comes only from the in-app/resume,agent-session-runtime.ts:218) — and"startup"is not in pi's replacement allow-listvaluestays on the old sessionHERDR_PANE_ID(src/pane.rs:141-152); the nested one setsrootSession = truefor itselfvaluemay be the inner pi's sessionpersist/restore.rs:499-545); duplicates dropped (restore.rs:754-781)valuepresent with no live process behind it; or absent on the duplicate paneclear_agent_runtime_identity_after_respawn(state.rs:2049-2068)agent_sessionabsentstate.rs:1260-1268,1535-1545)agentnames the other agentplannotator-tui --version(d) and (e) are the same shape and both produce "an exact session that is not the one in
the pane". The reporter's scenario 2 — several pi sessions in one folder, annotating from
the pane running the older one — matches (e) most closely:
pi -rre-anchoring isrejected when the pane already holds a pi ref. Note (a)-(c) and (g)-(h) all lose the
ref rather than corrupting it, and those land in the fallback path this PR fixes.
One thing worth knowing about the fallback fix's reach: pi does not touch a session
file's mtime on resume alone (
_setSessionFileonly reads for a well-formed currentfile,
session-manager.ts:940-962), and it does not create the file at all until thefirst assistant message (
_persist,:1074-1094). So mtime ranking becomes correct assoon as the resumed session is used, not at the instant it is resumed.
3. Not done here, deliberately
We could reject an exact path whose session header
cwddisagrees with the pane's cwdand fall back to discovery with a note. That would catch (d), (e) and (g) when the stale
session is in another folder — but not when it is in the same folder, which is the
reported case — and it would misfire on
--session-dir, subdirectory starts andsymlinked cwds. Not worth the risk without a reproduction.
refs plannotator/herdr-annotate#37