Skip to content

The Sessions module owns its Resume rules, tested in-process behind a seam #407

Closed

Description

@JacobStephens2

What to build

The Resume rules for agent sessions get tested in-process, behind a seam for their outside effects. This follows the same pattern as the Claim, Delivery, Run start, CI watch and Base fix seams.

The rules are:

  • a session that ends with background work killed gets one Resume;
  • a Resume that ends the same way gets no second Resume, and a progress line says it is carrying on;
  • a session with no id to resume is treated the same way;
  • a failed session is not resumed;
  • the last log is the Resume's;
  • a later failure names the killed work ahead of its own cause.

Today these rules sit in the same module as the code that spawns the Harness's CLI, follows its stream into the Session log, and stops it on an interrupt. So the only way to reach them is to run the thirdshift binary against fake claude/codex scripts on PATH.

After this change the Sessions module keeps the rules and reaches the machine through one private seam. That seam has two adapters: the real one, which spawns the CLI, and a scripted one in its tests. Behaviour seen from outside stays the same. A Run, a Repair, an Architect run and the progress lines and Session logs they produce do exactly what they do now.

Acceptance criteria

  • The Sessions module declares a private Outside trait with two methods, in the style of the other seams (&mut self, progress lines handed on as String):
    • run one session: given its kind, an optional session id to resume, its prompt and its Session log's path. It returns what the session ended with (session id, descriptions of killed background work, final message) or fails as a session fails today.
    • hand on a progress line.
  • What a session ended with is a small owned value, not the transcript reader. The real adapter builds it from the reader once the CLI has exited.
  • The rules (Resume once, carrying on after killed work, recording endings with killed work, the last log, and naming the killed work in a later failure) reach the machine only through Outside. They print no progress line directly. The "logging the session to …" line is handed on through it.
  • The real adapter holds the worktree and the Command's Harness choice. It does what a single session does today: builds the CLI arguments, picks the transcript reader, spawns the CLI in its own process group, follows the stream into the Session log, prints its condensed lines and the session-ended line, and stops the session on an interrupt with the Harness's stop signals.
  • The public interface of the Sessions module is unchanged for its callers (Delivery and the Architect run): the same "within" entry point, with linking the Factory skills and dropping a last log that was never created staying in that entry point; the same run and run-to-final-message methods.
  • In-process tests drive the rules through a scripted Outside that records each session call (kind, resume id, prompt, log path) and each progress line. They cover at least:
    • a clean session: one call, no Resume, its final message returned;
    • killed work with a session id: exactly one Resume, as <kind>-resume, with that id and the Resume prompt naming the work, plus the "resuming it once" line, returning the Resume's final message;
    • a Resume that also ends with killed work: no second Resume, the "the Resume … carrying on" line, and a later failing step's error naming "<kind> session ended with … and a later step failed";
    • killed work with no session id: no Resume, and the "the session … carrying on" line;
    • a failing session: its error is returned unchanged, with no Resume and no ending recorded;
    • steps that succeed after killed work: their result is not changed;
    • two sessions both leaving killed work: their endings are named oldest first, joined by "; ";
    • the last log is the Resume's path when there was a Resume.
  • Replace, don't layer: delete the end-to-end tests in tests/background_work.rs whose behaviour the in-process tests now cover. Those are the no-session-id warning, the Resume ending the same way, the later failure naming the work, and a Repair being resumed too. Keep one end-to-end Resume test per Harness (the Claude one in tests/background_work.rs, and the Codex ones in tests/codex_sessions.rs that check its resume argv). Also keep the tests that depend on Delivery, the Failed run path or the transcript readers: a merge refused after killed work, and a session ending in an error not being resumed.
  • cargo test and cargo clippy pass. No change to CONTEXT.md or an ADR is needed.

Blocked by

  • None (can start immediately)

Decisions

  • Take the Resume rules, not the whole Harness, as the deepening. A wider candidate was weighed: one Harness adapter owning every Claude/Codex fact (argv, sigil, skills directory, transcript reader, stop signals). This seam is the smaller first step, and it is the place such an adapter would later sit behind. The Resume rules (Resume and interrupt Codex sessions #395) are the most recently changed logic with no in-process test.
  • The seam is private to the Sessions module, named Outside. The repo's other in-process seams (Claim, Delivery, Run, CI watch, Base fix, Spec run) all use a private trait called Outside. Callers never see it, so Delivery's own seam, which stubs a whole session, stays as it is.
  • Two methods only: run one session, and hand on a progress line. Nothing else in the rules touches the machine. An interrupt is not part of the seam: it stops a running process, which only the real adapter has, and it reaches the rules as the session's error ("interrupted"), the same as today.
  • The rules choose the Session log's path and pass it to the adapter. The last log has to be recorded before the session runs, so a session that fails still hands its log to the Failed run and the Run notification. If the adapter chose the path, a failing session would never report it. The path comes from the existing Session-log naming, which is safe in tests: the command's start stamp is made on first use.
  • What a session ended with is an owned value (session id, killed work, final message). It replaces the boxed transcript reader handed back today. The rules read only those three things, and a scripted adapter can build them without implementing a transcript reader. The summary, failure flag and error stay inside the real adapter, which already turns them into the session-ended line or the session's error.
  • The Sessions value holds its adapter boxed in a RefCell. run and run_to_final_message take &self, as their callers rely on, and the struct already uses RefCell for its last log and endings. This matches the &mut self style of the other seams' traits.
  • Linking the Factory skills and the "log exists" filter stay in the public entry point. Both are thin filesystem effects outside the rules. The rules' own entry point, which the tests call, returns the last log unfiltered.
  • Tests: replace, don't layer. End-to-end tests of pure rules go. End-to-end tests that cover wiring (one Resume per Harness, with its argv) or other modules' behaviour (Delivery, the Failed run, the transcript readers) stay.
  • No glossary or ADR change. Resume and Session log are already defined in CONTEXT.md. Adding a seam the way the repo's other seams are added is neither surprising nor hard to reverse.

Architecture review

Strength: Strong · Dependencies: local-substitutable (a scripted adapter in-process, the real CLI in production)

Files: src/session.rs, src/progress.rs (the Stream trait it reads), tests/background_work.rs, tests/codex_sessions.rs

Before

flowchart LR
  Delivery --> Sessions
  Architect[Architect run] --> Sessions
  subgraph Sessions[Sessions module]
    direction LR
    rules[Resume rules] --> spawn[spawn CLI + follow stream + stop]
  end
  spawn --> CLI[claude / codex]
  Tests[e2e tests: fake CLI on PATH] -.only way in.-> CLI
  classDef leak stroke:#dc2626,stroke-width:2px;
  class spawn,Tests leak
Loading

After

flowchart LR
  Delivery --> Sessions
  Architect[Architect run] --> Sessions
  subgraph Sessions[Sessions module]
    direction LR
    rules[Resume rules] --> Outside{{Outside seam}}
  end
  Outside --> Real[real adapter: spawn CLI + follow + stop]
  Outside --> Scripted[scripted adapter]
  Real --> CLI[claude / codex]
  Tests[in-process tests] --> Scripted
  classDef deep stroke-width:3px;
  class Sessions deep
Loading

Problem: the Resume rules are mixed into process spawning, so the only way to test them is end to end, against fake CLIs.

Solution: the rules reach the machine through one private seam with a real adapter and a scripted adapter, as the other seams do.

Wins

  • Tests hit the rules in-process
  • Locality: Resume rules in one place
  • Two adapters justify the seam
  • Delete four slow end-to-end tests

Activity

  1. added
    needs-triageMaintainer needs to evaluate this issue
    ready-for-agentFully specified, ready for an AFK agent
    architect-planAn Architect plan: the Spec or Ticket an Architecture review published
    in-progressA Claim: thirdshift has taken this issue
    and removed
    needs-triageMaintainer needs to evaluate this issue
    ready-for-agentFully specified, ready for an AFK agent
    on Oct 5, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    architect-planAn Architect plan: the Spec or Ticket an Architecture review published

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions