Skip to content

refactor(tui): delete the disk-reading session code (ADR1 PR 3) - #24

Merged
zy1233 merged 6 commits into
mainfrom
refactor/adr1-drop-session-disk-reads
Oct 18, 2026
Merged

zy1233 merged 6 commits into
mainfrom
refactor/adr1-drop-session-disk-reads

Conversation

@zy1233

@zy1233 zy1233 commented Oct 16, 2026 •

Copy link
Copy Markdown
Owner

Summary

PR 3 of ADR1 ("pure ACP"). It was stacked on #23. #23 is merged (squash commit 7e0ca5a) and the base of this PR is now main. The head has one merge commit (git merge -s ours origin/main) so that the diff shows this PR only: the tree of the head is the same as the tree that CI tested before (5cc7f2f). #23 made zypi --continue, --resume and --session-id ask the agent. Nothing calls the old disk readers now. This PR deletes them and adds a gate that keeps them out. The pager reads no session file any more.

 pi-pager, pi-pager-bin (production code)
-  materialize_startup + helpers        reads <home>/sessions/** for the three flags
-  pin_local_resume_target, saved_resume_profile
-                                       a resumed session kept the sandbox saved with it (decision 2)
-  select_listed_by_title, PinnedResumeTarget      title match against the old summaries
-  extract_first_user_prompt            read chat_history.jsonl for the picker
-  resolve_local_session* checks        before a picker load
-  plan_file_path + disk fallbacks      read plan.md for the plan preview (decision 7)
-  restore_session_from_remote, plan_remote_miss   remote-restore branch of the old start-up
-  line_viewer::open_markdown(path), GrokHomeFixture   no caller left
+ scripts/tui_baseline.py, .toml: gates.session_disk_reads
+   8 substrings (list_summaries, resolve_local_session, session::storage, join("sessions"), ...)
+   scan of production code only: test files, #[cfg(test)] modules and comments are skipped

Behaviour that changes (decisions 2 and 7 of the plan, answered by the owner on 2026-10-16):

resumed session, sandbox   was: the profile saved with the session won; `--sandbox X` against a saved `Y` was refused
                           now: --sandbox, GROK_SANDBOX and the config decide, as for a new session
                           (the Python agent never saved a profile: nothing changes for its sessions)
plan preview               was: read <home>/sessions/.../plan.md when the request carried no content
                           now: shows the content of the request only

Evidence

  • Before: the new gate, pointed at main (12349d3): 18 hits. Pointed at feat(tui): resolve the start-up session by asking the agent (ADR1 PR 2) #23 (506769b): 17 hits.
    After: 0 hits. python scripts/tui_baseline.py gates passes. 6 new pytest functions (10 cases) test the scanner: a hit shows path and line; test files, comments and inline test modules are skipped; a #[cfg(test)] on one item hides nothing after it; the manifest is well formed; this tree has 0 hits.
  • Size: Rust +149 / −1,829 lines (production code +86 / −1,372). 27 tests go with the code (26 passed, 1 ignored): pi-pager 5,675 → 5,649.
  • tui_baseline.py check on this head (WSL2): all gates pass, 0 warnings, cargo check --workspace --tests clean. The first run on an earlier version had 1 warning: open_markdown lost its last caller. It is deleted.
  • Tests (WSL2, PI_E2E_REQUIRED=1): the whole manifest on 9600cdf (the Rust code of this head; the later commits change docs only: the plan, the baselines, the changelog and one user guide page): 8,705 passed, 1 known failure, 21 ignored, exit 0, 14 m 18 s. pi-pager 5,649 passed (12 ignored), pi-pager-bin 21. The known failure is the pi-pager-render tmux probe (known_failures in the manifest: it fails when the machine stalls, mostly on the first run after the test binary is linked). The other nine suites equal their reference_passed. The reference_passed of pi-pager is now 5,649 (the sum over the ten suites: 8,706).
  • CI (head 5cc7f2f): TUI CI run 38899203293: 8,706 passed, 0 failed, 21 ignored (the sum of reference_passed); pi-pager 5,649, and the tmux probe passes on the runner. cargo test job 16 m 24 s, Gates + cargo check 3 m 2 s. CI (Ruff, pytest 3.11, 3.12, 3.13) passes. The same numbers came from the first run on 79e01ee (run 38896545799). The final head 43c9a88 (the merge commit; same tree) gives the same again: TUI CI run 39141385047 8,706 passed, 0 failed, 21 ignored (cargo test 16 m 2 s, Gates + cargo check 3 m 34 s), and CI run 39141385046 passes.
  • Real zypi under a pseudo-terminal: resume_matrix.py --strict 13 PASS, 0 GAP, 0 FAIL (the old disk code is gone, so every case goes through the agent). smoke.py 23/23, exit_matrix.py 12/12, print_exit.py, mode_probe.py, status_line_probe.py 4/4, net_audit.py --mock 13/13: all exit 0.
  • Python: ruff check . and ruff format --check . pass. Full pytest on Windows: 2,531 passed, 39 skipped, 31 deselected (5 m 50 s). On Linux (WSL2): 2,540 passed, 30 skipped, 31 deselected (1 m 42 s).
  • Docs: CHANGELOG.md, the embedded user guide (17-sessions.md), packages/pi-agent-cli/AGENTS.md, the plan (r13, decisions 17 and 18 are new), the execution log (section 10.16, B.8), the baselines.
  • Found while measuring, not fixed here: the cost of reading all pages grows faster than the number of sessions, because the agent lists its directory again for each page. session_list_bench.py (WSL2 / Windows, median of 3): --session-id (walk all) 1.3 s / 1.8 s at 1,000 sessions and 28 s / 34 s at 5,000 (over the 30 s limit on Windows); --continue 0.6 s / 0.8 s at 1,000 and 10 s / 12 s at 5,000. A prototype that lists names only (not committed) takes the Windows number at 5,000 sessions from 34 s to 3.6 s. It is plan decision 18, on the Python side.
  • Not solved: a session that never got a message is listed as (no messages), and --continue can take it (plan decision 17: the agent should write the file at the first message). Decision 10 (delete the old store layer in pi-shell) is not in this PR.
  • Not verified: macOS and Windows builds of zypi; a real model turn after a resume (the e2e tests and the PTY scripts use the mock model).

Merge Danger

Door: two-way

Code deletion plus a gate and docs. A revert brings the old code back, and #23's path stays in use.

Blast Radius: start-up of --continue, --resume and --session-id; the sandbox choice of a resumed session; the plan preview

The sandbox change is visible only for sessions that carry a saved profile, which the old Rust agent wrote. The Python agent does not write one. The plan preview shows less than before when an approval request has no content (it showed the file on disk). The gate runs in gates, in check (so in TUI CI) and in pytest: a later change that reads session files in pi-pager or pi-pager-bin fails CI.

zypi looked for the session of --continue, --resume and --session-id in the
session files of the old Rust agent. The Python agent writes another layout, so
five cases of scripts/tui_pty/resume_matrix.py did not hold. Now zypi starts the
agent first and asks it over ACP: session/list (page after page) and
session/load. It reads no session file for these flags.

- acp/session_list.rs: one page walker (cursor, repeated cursor, page cap) for the
  /resume picker and for the start-up lookup.
- app/session_resolve.rs: resolve_startup(). --continue and --resume without a
  value take the latest activity (decision 4). --resume <id> reads every
  directory and loads in the directory of the session. --resume <title> matches
  in this directory. --session-id reads every page and refuses a used id. A miss
  ends the run with exit status 1 (decision 5).
- app/mod.rs: run() resolves after the connect, within the connect limit. On a
  failure it restores the terminal and stops the agent, as a failed connect does.
- resume_matrix.py --strict: 13 of 13 pass. Cases 5 and 11 were known gaps, and
  case 8 now needs exit status 1.
- session_list_bench.py: a "walk all" column, the cost of the --session-id check.

The old readers of the session files stay for now. The next change deletes them
(plan decisions 2 and 7).
The pager no longer reads session files. The last change (PR 2) resolves
--continue, --resume and --session-id through the agent, so the old readers
have no caller left. This change deletes them and adds a gate that keeps them
out.

Deleted, in the production code of pi-pager and pi-pager-bin:
- session_startup.rs: materialize_startup and its helpers (the latest session,
  the id, the title and the session-id checks on disk, and the remote-restore
  branch).
- cli.rs and pi-pager-bin main.rs: the pin of the resume target and the saved
  sandbox profile. A resumed session takes the sandbox as a new session does
  (plan decision 2).
- session_title_resolve.rs: the match against the old summaries.
  select_by_title stays.
- effects/helpers.rs, effects/mod.rs, dispatch/session/load.rs: the first-prompt
  reader (chat_history.jsonl) and the local-session checks before a load.
- agent_view/plan.rs: the plan.md read of the plan preview (plan decision 7).
- line_viewer.rs open_markdown(path) and test_util.rs GrokHomeFixture: no caller
  is left.

Added:
- gates.session_disk_reads (scripts/tui_baseline.py and .toml): production code
  of both crates must not call the readers of the session store or name the
  sessions directory. The scanner finds 18 hits on main, 17 after PR 2, and 0
  here. It runs in `gates`, in `check` and in pytest.
- 6 pytest cases for the gate.

27 tests go with the deleted code (26 passed, 1 ignored): pi-pager 5,675 ->
5,649. The embedded user guide (17-sessions.md) and CHANGELOG.md say what
changed for the user.
The user answered decisions 2, 4, 5 and 7 on 2026-10-16, all as proposed.
This commit records the work of PR 2 (#23) and PR 3 (#24).

- Plan: status r13. Decisions 2, 4, 5 and 7 are marked done. Two new open
  decisions: 17 (the empty session) and 18 (the cost of paging). The 1.R1
  box is ticked. Decision 10 stays open.
- Execution log: section 10.16 (what changed, the flags, the 13 disk-read
  call sites, the 13-case resume matrix, the paging cost with numbers, the
  checks that ran, the limits), appendix B.8 (how to reproduce), revision r13.
- Baselines: new gate row `session_disk_reads`, pi-pager 5,649 and total
  8,706 in `tui.md`, one more paragraph in `tui-history.md`, and
  `tui-support.md` regenerated at 9600cdf.
- packages/pi-agent-cli/AGENTS.md: the TUI finds a session only through
  `session/list` and `session/load`. The note names the list fields it needs
  and says what reading all pages costs.
TUI CI on 79e01ee (run 38896545799): 8,706 passed, 0 failed, 21 ignored.
This is the sum of `reference_passed` in the manifest. `pi-pager` is 5,649
and the tmux probe of `pi-pager-render` passes on the runner.

Updated: `docs/baselines/tui.md` (tests and time lines),
`docs/baselines/archive/tui-history.md` (r13 paragraph), the plan (status
paragraph) and the execution log (section 10.16, checks).
Reading all pages of `session/list` costs pages x directory size, because
the agent lists its session directory again for each page. At 5,000 sessions
`--session-id` takes 28-34 s, which is over the 30 s limit on Windows
(plan decision 18).

The CHANGELOG and the embedded user guide (17-sessions.md) now say so. No
code changes.
zy1233 added a commit that referenced this pull request Oct 18, 2026
…2) (#23)

zypi --continue, --resume and --session-id now start the agent first and ask
it which session is meant: session/list (page after page) and session/load.
They read no session file for this. A session that does not exist ends the
run before the event loop starts: the terminal is restored, the agent is
stopped, the message is printed, and the exit status is 1.

The owner answered plan decisions 4 and 5 on 2026-10-16, both as proposed:
--continue takes the session with the latest activity, and an unknown id
fails before the UI starts. resume_matrix.py --strict is 13 of 13 (it was
11 of 13). The old disk readers stay until the next PR (#24).

TUI CI on the head (506769b): 8,732 passed, 0 failed, 22 ignored.
@zy1233
zy1233 deleted the branch feat/adr1-resolve-startup October 18, 2026 15:36
@zy1233 zy1233 closed this Oct 18, 2026
@zy1233 zy1233 reopened this Oct 18, 2026
@zy1233
zy1233 changed the base branch from feat/adr1-resolve-startup to main October 18, 2026 15:37
@zy1233
zy1233 merged commit 0476ea4 into main Oct 18, 2026
7 checks passed
@zy1233
zy1233 deleted the refactor/adr1-drop-session-disk-reads branch October 18, 2026 16:01
zy1233 added a commit that referenced this pull request Oct 19, 2026
…n 17) (#25)

* feat(agent): write a session file with its first message (plan decision 17)

A new session was a file from session/new on: a header and nothing else. A
user who started the TUI and left, left such a file behind. session/list
showed it as "(no messages)", and zypi --continue could take it for the
latest session.

session/new now makes a deferred session (DeferredJsonlSessionStorage, in
the harness). It keeps its entries in memory until the first message entry.
That entry writes the header, the entries held before it (a model choice, a
name, a leaf move) and itself in one go. After that each entry is appended
as before. The write comes before the entry is recorded, so a failed write
leaves no trace of the message and the next message tries again. A session
in which nobody typed has no file: nothing to list, load or continue.
Sessions that older versions left with no message keep the title
"(no messages)".

An id the client chose (--session-id) must stay taken while its session has
no file, so the agent keeps the ids of its open sessions in a set. The set
leaves an id when the session is closed or deleted, or when the session
could not be opened. Another process cannot see that set. So the first
prompt of a session with a chosen id looks for the id in the files once
more, and refuses it with the same error as session/new if a session has
saved under it since.

session/load and session/resume answer "not found" for a session that has no
file: there is nothing to load.

* test(tui): check the late session file at the TUI level (plan decision 17)

The agent now writes a session file with the first user message. Three
checks that drive the real pieces had to change with it:

- scripts/tui_pty/resume_matrix.py (zypi under a pseudo-terminal): the TUI
  makes a session at once, with no message, for `--session-id` and for
  `/new`. (A start with no flag makes none until the first message.) Two new
  cases leave such a session with no message and require that no session
  file exists. The first one comes first on purpose: under an agent that
  writes at once, its empty session is the newest one, so `--continue` and
  `--resume` with no argument take it and fail too. Two cases type a message
  and look for the file after it: `--session-id <fresh uuid>`, and `/new`
  and then a message. A case can have steps (`Step`). wait_for_run types the
  first one when the TUI is up (the model name on the border of the input
  box), and each next one when the screen shows that the one before took
  effect. The `/new` cases start with `--resume <id>`, so they do not depend
  on which session is the newest. scripts/tests/test_tui_pty.py tests the
  cases and wait_for_run with a fake terminal.
  Mutation check: with either of the two `repo.create` calls in agent.py
  made to write at once, the case for that call fails (and for the call that
  serves `--session-id`, the two cases that take the newest session fail too).
- scripts/tui_pty/smoke.py: after a restart, the /resume picker used to
  list the new empty session first and the script pressed Down to skip it.
  That session has no file now and is not listed, so the earlier session is
  the first row and the script no longer presses Down.
- python_agent_e2e.rs (the Rust ACP client against the real Python agent):
  the sessions that must be listed (51 for the paging test, the `existing`
  one for the id check) now get a first message. The id check also asserts
  what the agent does for a session that has no file yet: it refuses a
  second `session/new` with the same id (InvalidParams), and once the first
  message is sent the pager's own check finds the id through session/list.

No test is added or removed on the Rust side (the count of
python_agent_e2e stays 14).

* test(agent): close two gaps that a mutation run found (plan decision 17)

A run of 15 one-line changes to the production code of this PR, each
against the tests that watch it, found two that no test noticed:

- The header of a deferred session took its time from a second look at
  the clock and not from the time in the file name. The test compared the
  two, but both looks fell in the same millisecond, so it passed by
  chance. It now gives every look at the clock a new time.
- The second look at a chosen id (before the first message) also refused
  a session whose own file was there. No test sent a second message in a
  session with a chosen id. A new test sends three.

After the change, both are caught. The other 13 were caught before.

* docs: record plan decision 17 (the session file is written at the first message) and r14

- CHANGELOG.md, [Unreleased]: the agent writes the file of a session with
  its first message. It lists the behaviour changes (no flag: no session
  until the first message; `--session-id` and `/new`: a session at once,
  with no file; a chosen id stays reserved while its session is open),
  what library users get (`JsonlSessionRepo.create({"deferred": True})`),
  and the tests. The entry for the cost of paging says it is left as it is.
- The embedded user guide (17-sessions.md), packages/pi-agent-cli/AGENTS.md
  and docs/DESIGN.md section 6.1: a session in which nobody typed has no
  file, so it is not listed and cannot be loaded.
- scripts/tui_pty/README.md (16 cases) and docs/baselines/tui.md.
- The plan (r14): the status, the answers to the four questions (#23 and
  #24 merged, decision 18 not done, decision 17 done, decision 10 to do),
  and the index of section 10.
- The execution log: section 10.17 (the merges, the design of decision 17,
  what each ACP method does for a session without a file, the facts about
  the TUI, the tests, two mutation runs and the two gaps they found, the
  numbers, what is not verified), B.9 (how to repeat it) and the revision
  record. One old line (about the empty first row of the /resume picker)
  points to section 10.17.
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.

1 participant