Skip to content

fix(harness): track session input delivery [Agent Map 04/15] - #823

Merged
ynadge merged 1 commit into
mainfrom
review/agent-map-04-session-input
Sep 6, 2026
Merged

fix(harness): track session input delivery [Agent Map 04/15]#823
ynadge merged 1 commit into
mainfrom
review/agent-map-04-session-input

Conversation

@ynadge

@ynadge ynadge commented Sep 5, 2026

Copy link
Copy Markdown
Contributor

Primary change type

  • Bug fix

Problem and motivation

Writing text or Enter to a terminal does not prove the intended live coding process accepted a turn. Process replacement and manual input can race programmatic delivery.

Summary and scope

Track text and submission phases, fence delivery and status by the current runtime identity, isolate composer input, and contain interrupted or failed writes during shutdown and WebSocket input.

Recheck terminal input ownership after the durable pre-write hook and compensate an unsubmitted claim when manual input wins the race. The same compensation applies to staged-input collisions.

How this increment fits

Ordinary session input and runtime fencing are complete here. Bootstrap and delegation consume this delivery contract in later parts.

Stack and review boundary

  • Part 04 of 15 in the Agent Map review stack; review this increment against its predecessor.
  • Base: review/agent-map-03-map-navigation.
  • Current head: 3d105bcac3517691cefb893382a54633204f8229; 2,228 changed lines across 11 files, counting additions and deletions including tests.
  • Repackages the corresponding final behavior from #804. Original code and review history remain preserved.
  • Complete coworker testing branch: fix/studio-onboarding-followups.
  • The stack remains unmerged. Dependent PRs target their predecessor, so their diffs do not repeat earlier increments.

Related work

Agent Map checkpoint SAP-3147; relevant work SAP-3148. This packaging follows the maintainer-approved 15-PR split.

Validation

Root checks ran against 023c09d98991ba60cec88f60b0aaf443a99681da. The final head changes only README terminology or commit ancestry; a complete tracked-file comparison confirms identical executable source and build inputs. The terminology gate was rerun on 3d105bcac3517691cefb893382a54633204f8229.

pnpm build — passed (exit 0)
pnpm typecheck — passed (exit 0)
pnpm lint — passed (exit 0)
pnpm test — passed (exit 0)

Tests and documentation

Regression coverage: Partial writes, text/Enter acknowledgement, runtime replacement, stale status events, manual-input preemption during durable writes, and interrupted shutdown.

See part 15 for integrated browser, native CLI, and Mac journey validation. The checks above were run independently on this PR’s own commit.

Linux tests run with ordinary user filesystem permissions; the sandbox's extra ambient capabilities are dropped. Hosted CI and automated review are separate from these recorded local results.

Compatibility and release impact

  • Compatibility: Breaking for embedders: SessionManager.write() can throw SESSION_INPUT_ISOLATION_REQUIRED when prior partial input cannot be cleared. Terminal-forwarding callers must handle the failure.
  • Changeset: Included: .changeset/ordinary-session-input.md

Security

  • No secrets, credentials, private user data, or unsanitized logs are included.
  • This PR does not publicly disclose a suspected vulnerability.

AI assistance

  • Codex assembled the implementation, addressed reproduced defects, supplied tests and documentation, inspected the diff, and ran the checks above. Reviews are handled by hosted PR automation.

Checklist

  • Read CONTRIBUTING.md; implementation follows the requested 15-PR split.
  • Description reflects this PR's actual predecessor-relative diff.
  • Relevant tests accompany the changed behavior.
  • Root build, typecheck, lint, and test evidence matches the final implementation; any documentation-only update is identified above.
  • Release/documentation treatment is explained above.

@github-actions

github-actions Bot commented Sep 5, 2026

Copy link
Copy Markdown
Contributor

Review — PR #823 (Agent Map 04/15, session input delivery)

No confidentiality findings: the changeset, comments and test fixtures are provider-neutral and
name no customer, partner or internal system. No frontend component code in the diff.

1. Raw keystrokes during beforeFirstWrite are not preempted — the exact prompt-merge this PR exists to prevent

packages/harness/src/core/session-manager.ts:1571-1595

The background-preemption fence reads terminalInputEpochs exactly once, at line 1571, and is
never re-read. Immediately after it comes await lifecycle.beforeFirstWrite() (line 1586) —
documented as "durable transition that must commit before the first PTY byte", i.e. an unbounded
I/O wait; the new test "waits for the durable pre-write hook before crossing the PTY boundary"
holds it open with a deferred to prove exactly that. The only fence after that await is
this.closing || canWriteNow() (line 1592), and canWriteNow is caller-supplied and knows
nothing about terminal input. stagedInputs has no entry yet either, so the write() preemption
path at line 1478 cannot fire.

Failure scenario: a background submitInput(id, "automatic map bootstrap", true, undefined, true, { beforeFirstWrite }) passes the epoch check; while beforeFirstWrite is committing, the user
types fix the login bug into the same live composer. write() bumps the epoch, but nobody reads
it. submitInput resumes, writes its text onto the user's line, sleeps SUBMIT_DELAY_MS, sees
staged.preempted === false, and writes \r. The agent receives
fix the login bugautomatic map bootstrap as one turn.

Fix: re-check (this.terminalInputEpochs.get(id) ?? 0) !== initialTerminalInputEpoch in the
synchronous fence at line 1592, alongside this.closing (and route it through
onNotSubmitted/SessionBackgroundInputPreemptedError(false) like the other pre-write bailouts).

2. SessionManager.write() gains a routine throw, shipped as a patch with no note and no exported error type

.changeset/ordinary-session-input.md:5, packages/harness/src/core/session-manager.ts:1458

SessionManager is published type surface: src/index.ts exports HarnessServer, whose
sessionManager: SessionManager field puts every method here in dist/index.d.ts. write()
previously returned boolean on a fenced composer; it now throws SessionInputIsolationError as
an ordinary, expected outcome — this PR had to add a try/catch in terminal-ws.ts precisely
because nothing was catching it before. setReady() also silently changed to no-op on exited
sessions, and submitInput() grew two positional parameters.

An embedder that calls server.sessionManager.write(...) from a synchronous socket/IPC handler now
gets an uncaught exception on a code path that used to be total. That is a behavior change a pinned
consumer breaks on, described in the changeset only as "handled consistently".

Two fixes, both needed:

  • Bump the changeset to minor and state the new throw explicitly, with the "catch and reconnect"
    guidance terminal-ws.ts:88-96 already encodes.
  • Export SessionInputIsolationError (and SessionManagerClosingError,
    SessionBackgroundInputPreemptedError) from src/index.ts next to the existing HarnessError
    block. As written they cross the published boundary but are unreachable by name, so consumers can
    only string-match err.code — the same gap SessionInputGuardRejectedError already has; don't
    widen it.

3. The staged-collision bailout is the one pre-Enter reject that skips onNotSubmitted

packages/harness/src/core/session-manager.ts:1649-1651

Every other bailout before the first byte compensates the caller: the isolation reject (1581), the
beforeFirstWrite throw (1588), and the final fence (1593) all await onNotSubmitted(). The
stagedInputs.has(id) collision throws bare — and it sits after beforeFirstWrite has already
committed its durable transition, so the caller is left with a committed pre-write record and no
positive not-submitted evidence to unwind it. A caller using submitInput directly (rather than
submitInputTracked, which infers phase: "not-written") cannot distinguish this from an
ambiguous write and, per this PR's own contract, must fail closed and drop the turn.

Also note the error is SessionBackgroundInputPreemptedError even when the loser is a foreground
API submission and nothing was preempted — the collision is rejected, not the incumbent.


Verdict: Request changes — finding 1 is a live prompt-corruption path in the exact invariant
this increment is written to establish, and finding 2 ships a breaking behavior change to a
published API as a patch.

@ynadge
ynadge force-pushed the review/agent-map-03-map-navigation branch from 3da297c to 154c3e8 Compare September 5, 2026 11:57
@ynadge
ynadge force-pushed the review/agent-map-04-session-input branch from d824dc0 to 023c09d Compare September 5, 2026 11:57
@github-actions

github-actions Bot commented Sep 5, 2026

Copy link
Copy Markdown
Contributor

Follow-up review — PR #823 (delta since d824dc0)

Own-PR delta is one commit, 023c09d9: session-manager.ts (+14), its tests (+55), and the
changeset. Everything else in the range is the rebased base branch (#822). No confidentiality
issues in the new prose; the changeset stays provider-neutral.

Fixed

  • Finding 1 (prompt merge during beforeFirstWrite) — resolved. session-manager.ts:1596-1604
    re-reads terminalInputEpochs after the hook, before the first byte, with no await between
    that check and pty.write, and routes the bailout through onNotSubmitted +
    SessionBackgroundInputPreemptedError(false). Covered by "preserves user keystrokes received
    during a deferred pre-write hook".
  • Finding 3 (staged collision skipped compensation) — resolved at
    session-manager.ts:1663, with a test.

Not fixed

  • Finding 2, second half — error classes still unexported. The changeset now correctly says
    minor (0.14.0, pre-1.0) and calls out the write() throw as Breaking, but
    SessionInputIsolationError, SessionManagerClosingError and SessionBackgroundInputPreemptedError
    (session-manager.ts:589/599/610) are still absent from packages/harness/src/index.ts, while
    SessionManager reaches dist/index.d.ts via HarnessServer.sessionManager
    (server/index.ts:375). The changeset tells embedders to "handle this failure" using a name they
    cannot import — the only option is string-matching err.code. Add them to the export block
    alongside startServer.

Nit

  • The two bailouts before beforeFirstWrite — the canWrite reject (:1568) and the first epoch
    fence (:1576) — still throw bare, while every bailout after them awaits onNotSubmitted. Same
    inconsistency the new fix removed one line lower.
  • Correction to round 1: the SessionBackgroundInputPreemptedError-for-foreground-collision remark
    was overstated; the error describes the losing caller, and it is only mislabelled when that
    loser was itself a foreground submission.

Verdict: Approve once the three error classes are exported. The correctness fix is sound.

@ynadge
ynadge force-pushed the review/agent-map-03-map-navigation branch from 154c3e8 to cf2369c Compare September 5, 2026 12:17
@ynadge
ynadge force-pushed the review/agent-map-04-session-input branch from 023c09d to 3d105bc Compare September 5, 2026 12:17
Base automatically changed from review/agent-map-03-map-navigation to main September 6, 2026 22:20
@ynadge
ynadge merged commit dae5fc0 into main Sep 6, 2026
1 check passed
@ynadge
ynadge deleted the review/agent-map-04-session-input branch September 6, 2026 22:20
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