Skip to content

Commit 8e9068a

Browse files
DDKingerCopilot
andauthored
Remove the dead history_loader module (#686)
* Remove the dead history_loader module `history_loader::load_all` was deleted when host history moved to ACP `session/list`, but the module stayed behind: half of it was disk-scanner dead code kept compiling only by `#[allow(dead_code)]`, and its header still documented a per-CLI on-disk scan that no longer runs. The name alone implied WTA cold-scans four CLI transcript stores at startup. Dissolve it into the code that actually uses the survivors: - `parse_iso_to_system_time` and `short_id` move to `session_history.rs` next to their only callers (ACP `SessionInfo::updated_at` and the no-title fallback label), with the Codex-specific doc rewritten. - `codex_record_is_subagent_meta` / `codex_payload_is_subagent` move to `session_watcher/classify_codex.rs` as `record_is_subagent_meta` / `payload_is_subagent`, beside the classifier that consumes them. - Delete the YAML block-scalar parser, the JSONL title extractors and their helpers, all unused. Also refresh comments that still pointed at `load_all` or the removed `wsl_acp` module, and mark the two spec sections describing the deleted disk scanner as superseded. No behavior change; tests move with their code. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 741e2eec-21f3-4286-8e2e-500eb06d553d * Parse timestamps with the time crate instead of by hand `parse_iso_to_system_time` was ~125 lines of hand-rolled ISO 8601: manual offset scanning, a leap-year table, day-of-month validation, and its own days-since-epoch arithmetic. Replace it with `time::OffsetDateTime` + `Rfc3339`. `time` v0.3.47 is already linked into wta through ratatui-widgets and tracing-appender and is already listed in cgmanifest.json and NOTICE.md, so promoting it to a direct dependency adds no third-party code; only the `parsing` feature is new. Re-running Generate-WtaThirdPartyNotices.ps1 produces no diff (276 crates before and after). Behavior is preserved, including the two non-RFC-3339 quirks the old parser had, now explicit and tested: - offset-less `YYYY-MM-DDTHH:MM:SS` is still read as UTC, via a retry with `Z` appended rather than silently dropping the value; - pre-epoch timestamps still return `None`, so callers can keep treating `SystemTime` as an offset from `UNIX_EPOCH`. All existing parse tests pass unchanged; added cases for the offset-less fallback and for malformed input. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 741e2eec-21f3-4286-8e2e-500eb06d553d * Address review: spelling and helper visibility - Rename `parse_iso_reads_offsetless_timestamps_as_utc` to `..._without_offset_as_utc`. check-spelling flagged `offsetless`; rewording keeps the allowlist untouched for a single test name. - Narrow the Codex subagent helpers moved in this PR: `record_is_subagent_meta` is `pub(super)` (its only caller is `session_watcher::process_change`) and `payload_is_subagent` is now private, since nothing outside `classify_codex` calls it. `classify` keeps its existing visibility; it predates this PR. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 741e2eec-21f3-4286-8e2e-500eb06d553d * Convert to SystemTime without a panicking Add `SystemTime::from(OffsetDateTime)` reaches `impl Add<Duration> for SystemTime`, which panics on overflow, so the function's documented "never panics" contract held only because `time` caps years at 9999. That cap is the `large-dates` feature, and feature unification puts it outside this crate's control, so fail closed instead: take `unix_timestamp()` through `u64::try_from` (which also subsumes the pre-epoch rejection) and add it with `SystemTime::checked_add`. Adds `parse_iso_preserves_fractional_seconds`, since the `Duration` is now assembled by hand and sub-second precision has to be carried across explicitly. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 741e2eec-21f3-4286-8e2e-500eb06d553d * Fix comments that misdescribed the agent-pane origin join `classify_and_map` does not stamp `AgentPane` origin onto historical rows — it consults the agent-pane index to subtract those sessions from history outright. Repointing the old `history_loader::load_all` comments at it carried over a claim that was true of the deleted disk scanner but is not true of the ACP mapper. - app.rs and the `SessionOrigin` doc now say the flag is stamped on the live row and kept after it ends, and that rows rebuilt from ACP `session/list` never carry it because they are filtered out. - session_registry.rs: `session_history` maps `session/list` rows; the listing itself happens in its caller. Dropped the "ACP history scan" wording that implied it owns I/O. - hybrid-agent-session-tracking.md: the superseded note now names the removed symbols the section still references, so readers do not go looking for deleted code. Comment-only; no behavior change. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 741e2eec-21f3-4286-8e2e-500eb06d553d * Attribute the overflow panic to the right impl `SystemTime::from(OffsetDateTime)` has no `Add` of its own; it is implemented as `SystemTime::UNIX_EPOCH + duration`, and that `Add` impl is what panics on overflow. Reword so the comment names the impl the `checked_add` is actually avoiding. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 741e2eec-21f3-4286-8e2e-500eb06d553d --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 741e2eec-21f3-4286-8e2e-500eb06d553d
1 parent 728bc46 commit 8e9068a

15 files changed

Lines changed: 208 additions & 728 deletions

doc/specs/hybrid-agent-session-tracking.md

Lines changed: 14 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -242,6 +242,18 @@ helper-side and unrelated).
242242

243243
### Title resolution (and the codex AGENTS.md fix)
244244

245+
> **Superseded.** The on-disk title scan described below was removed with the
246+
> rest of the disk scanner: titles now come from ACP `session/list`
247+
> (`SessionInfo::title`, mapped in `session_history.rs`, with
248+
> `session_history::short_id` as the fallback label). Only the codex
249+
> subagent-fork check survives, as
250+
> `session_watcher::classify_codex::record_is_subagent_meta`. The rationale
251+
> below is kept for history — every other file, function, and helper it
252+
> names (`history_loader.rs`, `try_refresh_title_from_disk`,
253+
> `lookup_title_for_session`, `codex_title_from_file`,
254+
> `codex_user_text_is_synthetic`, `codex_session_has_real_content`) no
255+
> longer exists in the tree, so do not go looking for it.
256+
245257
A watcher row is created with a **synthetic** title (cwd basename, or empty),
246258
then upgraded from the CLI's on-disk artefacts by `try_refresh_title_from_disk`
247259
`lookup_title_for_session`, the **same** disk-title path the hook and
@@ -283,7 +295,7 @@ title).
283295
| Apply / dedup / gate / reaper | `tools/wta/src/master/mod.rs` (`apply_watcher_event`, `handle_session_hook`, `ensure_watched_session_row`, `watcher_row_allowed`, `live_it_pane_guids`, `reap_dead_class_b_sessions`, `hook_owned` + `born_bound` sets) |
284296
| Born-bound registration | `session_registry.rs` (`build_born_bound_request`, `INTELLTERM_METHOD_SESSION_BORN_BOUND`), `main.rs` (`register_launched_session_with_master`) |
285297
| Row `bound_pid` field | `tools/wta/src/session_registry.rs` |
286-
| Codex title / subagent / phantom | `tools/wta/src/history_loader.rs` |
298+
| Codex subagent fork detection | `session_watcher/classify_codex.rs` (`record_is_subagent_meta`) |
287299
| User-input tool heuristic | `agent_sessions.rs` (`is_user_input_tool`) |
288300

289301
### Status detection (per-CLI)
@@ -403,7 +415,7 @@ of the live states.
403415
`session_hook_marks_*`; born-bound: `session_born_bound_marks_born_bound_not_hook_owned`,
404416
`born_bound_session_gets_watcher_activity_without_rebinding`,
405417
`real_hook_takes_over_born_bound_session`, `resume_binding_events_are_born_bound_not_hook_owned`),
406-
`history_loader::tests` (codex title / subagent / phantom), and
418+
`classify_codex::tests` (codex subagent fork detection), and
407419
`session_watcher` discovery/classify tests (incl. `classify_claude` turn-based:
408420
user→Working, `stop_reason` end_turn→Idle / tool_use→Working, streaming-partial
409421
stays Working, AskUserQuestion→Attention).

doc/specs/wsl-session-management.md

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,11 @@
11
# WSL Agent Session Management (Historical MVP)
22

3+
> **Superseded in part.** This spec is written against the on-disk history
4+
> scanner (`history_loader::load_all`) and the `wsl_acp` scan module, both of
5+
> which have since been removed. Host history now comes from the agent's own
6+
> ACP `session/list` (`session_history.rs`); there is no WSL history scan in
7+
> the tree today. The design rationale below is kept for history.
8+
39
## Abstract
410

511
Intelligent Terminal (IT) surfaces a list of agent-CLI sessions (Copilot /

tools/wta/Cargo.lock

Lines changed: 1 addition & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

tools/wta/Cargo.toml

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -61,3 +61,8 @@ sys-locale = "0.3"
6161
# (issue #287). Already in the lockfile as a transitive dep of clap, so
6262
# promoting it to a direct dep does not change the resolved graph.
6363
strsim = "0.11"
64+
# RFC 3339 parsing for ACP `SessionInfo::updated_at`. Already in the lockfile
65+
# as a transitive dep of ratatui and tracing-appender, so promoting it to a
66+
# direct dep does not change the resolved graph; only the `parsing` feature
67+
# is added on top of what those crates already enable.
68+
time = { version = "0.3", features = ["parsing"] }

tools/wta/src/agent_pane_origin.rs

Lines changed: 6 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -27,9 +27,8 @@
2727
// ENTER-routing work that will consume this field):
2828
// {"v":2,"session_id":"<uuid>","origin":"agent_pane","pane_session_id":"<WT pane GUID>","started_at":"<RFC3339-ish>"}
2929
//
30-
// We deliberately do NOT record `cli_source` — `history_loader` already
31-
// derives it from which per-CLI on-disk artefact directory the session was
32-
// found in, so duplicating it here would create a second source of truth
30+
// We deliberately do NOT record `cli_source` — the session's own agent
31+
// reports it, so duplicating it here would create a second source of truth
3332
// that could drift. Same rationale for `owner_tab_id`: no caller needs it
3433
// yet, and we can always recover it via WT itself.
3534
//
@@ -44,8 +43,8 @@
4443
// The file is append-only; it is never read-then-written from this module.
4544
// Old entries become orphans naturally when the corresponding CLI session
4645
// directory is deleted by the user or the agent CLI itself — orphan entries
47-
// in the index are harmless because `history_loader` only consults the
48-
// index when constructing rows for sessions that *still exist on disk*.
46+
// in the index are harmless because the index is only ever consulted as a
47+
// filter against session ids the agent itself still reports.
4948

5049
use std::collections::{HashMap, HashSet};
5150
use std::fs::{File, OpenOptions};
@@ -133,8 +132,8 @@ pub fn append_to(
133132

134133
/// Load the default index into a `HashSet<String>` of session ids. Empty
135134
/// set if the file does not exist, cannot be opened, or is empty — never
136-
/// errors out to the caller, which lets `history_loader` proceed even on
137-
/// a fresh install or after a manual delete.
135+
/// errors out to the caller, so the history scan still proceeds on a fresh
136+
/// install or after a manual delete.
138137
///
139138
/// Unpackaged dev binaries also merge the installed Intelligent Terminal
140139
/// package's LocalState index when present. That keeps diagnostics such as

tools/wta/src/agent_sessions.rs

Lines changed: 9 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -89,7 +89,7 @@ pub(crate) fn title_is_placeholder(cli: &CliSource, title: &str) -> bool {
8989
23 => *byte == b'Z',
9090
_ => byte.is_ascii_digit(),
9191
})
92-
&& crate::history_loader::parse_iso_to_system_time(timestamp).is_some()
92+
&& crate::session_history::parse_iso_to_system_time(timestamp).is_some()
9393
}
9494

9595
#[derive(Clone, Debug, PartialEq, Eq, serde::Serialize, serde::Deserialize)]
@@ -153,10 +153,12 @@ pub enum LivenessState {
153153
///
154154
/// Populated authoritatively by `agent_pane_origin`: WTA appends a record
155155
/// to the on-disk index whenever it creates an ACP session for an agent
156-
/// pane (i.e. `--owner-tab-id` was supplied), and `history_loader` joins
157-
/// that index when reconstructing historical rows. Live rows default to
158-
/// `Unknown` because the UI only surfaces this badge for ended/historical
159-
/// sessions, where it is most useful.
156+
/// pane (i.e. `--owner-tab-id` was supplied). A row is stamped while it is
157+
/// live, when a routed event's session id turns up in that index, and it
158+
/// keeps the flag once the session ends — which is where the UI surfaces
159+
/// it. Rows rebuilt from ACP `session/list` never carry it:
160+
/// `session_history::classify_and_map` uses the same index to drop
161+
/// agent-pane sessions from history entirely rather than to badge them.
160162
#[derive(Clone, Debug, Default, PartialEq, Eq, serde::Serialize, serde::Deserialize)]
161163
pub enum SessionOrigin {
162164
/// Origin not recorded — either the session pre-dates the index, was
@@ -1147,8 +1149,8 @@ impl AgentSessionRegistry {
11471149
/// is `Historical` or `Ended`, upgrade it to `Live` (`AgentStatus::Idle`)
11481150
/// and bind the pane.
11491151
///
1150-
/// Motivation: at startup the on-disk history scan
1151-
/// (`history_loader::load_all`) and the helper's `list_sessions`
1152+
/// Motivation: at startup the ACP `session/list` history scan
1153+
/// (`session_history::classify_and_map`) and the helper's `list_sessions`
11521154
/// bootstrap can land in either order, and a WTA process attached
11531155
/// to an existing master in another WT window may never see the
11541156
/// originating `SessionStarted` hook event. Without this join, a

tools/wta/src/app.rs

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -732,8 +732,10 @@ where
732732

733733
// Stamp `AgentPane` origin on the live session if the agent-pane
734734
// origin index recorded its session id. This is what flips the
735-
// "agent pane" prefix on for *live* rows — historical rows pick up
736-
// the same flag through `history_loader::load_all`'s join. We
735+
// "agent pane" prefix on for *live* rows. Rows rebuilt from ACP
736+
// `session/list` never get it: `session_history::classify_and_map`
737+
// consults the same index to subtract agent-pane sessions from
738+
// history outright rather than to badge them. We
737739
// re-read the index on every routed event (small file, infrequent
738740
// event) rather than caching, to stay correct after a new session
739741
// is created while wta is already running.

0 commit comments

Comments
 (0)