# PR Review Report: `fix/term5`

## Scope

- Branch: `fix/term5`
- Reviewed against: `main...HEAD`
- Related issue: nano-props/goblin#307
- Review method: static inspection by the primary reviewer and three parallel sub-reviewers
- No repository files were modified during this review.
- Tests, typecheck, and architecture checks were not run in this review round.

## Executive conclusion

Do not merge in the current state.

The primary xterm startup layout sequence is now well structured and substantially satisfies issue #307: the initial 80x24 buffer is hidden, fitting and full-buffer refresh happen before PTY attach, asynchronous render barriers are awaited, replay completes before reveal, and normal resize requests are protected by client epoch/runtime generation checks plus a server generation check.

However, two cross-generation mutation paths remain open:

1. A resize-triggered automatic takeover does not carry an expected runtime generation and can resize a replacement PTY using stale geometry.
2. A write that is already waiting for asynchronous authority authorization can cross a restart and be delivered to the replacement shell because write requests are not generation-bound.

These are lifecycle/data-boundary defects, not merely missing UI guards. They should be closed at the server mutation boundary before merge.

## Findings

### 1. Medium: automatic takeover bypasses resize generation protection

Relevant locations:

- `src/web/components/terminal/TerminalSession.ts` around the resize authorization path
- `src/web/components/terminal/authority-gate.ts` around `doTakeover()`
- `src/shared/terminal-types.ts` at `TerminalResizeInput` and `TerminalTakeoverInput`
- `src/server/terminal/terminal-session-manager.ts` at `resizeSession()` and `takeoverSession()`

Normal resize requests now carry `terminalRuntimeGeneration`, and `resizeSession()` rejects a generation mismatch. That protection does not cover a viewer resize that first promotes itself through takeover.

The problematic sequence is:

```text
old generation view requests resize
  -> AuthorityGate begins takeover without a generation
  -> session restarts and installs a new PTY generation
  -> stale takeover reaches the server with old view geometry
  -> takeover registers/claims the client and applies that geometry to the current PTY
  -> subsequent generation-bound resize is rejected, but the new PTY was already resized
```

`TerminalTakeoverInput` was split from `TerminalResizeInput` and contains only the runtime session ID and geometry. The server therefore cannot determine whether the takeover geometry belongs to the current PTY generation.

Recommended correction:

- Add an expected runtime generation to the generation-bound automatic takeover path.
- Validate it in the session manager before client registration, authority claim, or geometry mutation.
- If explicit user takeover intentionally has cross-generation semantics, model it separately or make the expected generation optional only for that explicit operation.
- Add a test that suspends takeover authorization, restarts the session, then resolves the stale takeover and proves the replacement PTY geometry is unchanged.

### 2. Medium: authorized writes can cross restart generations

Relevant location:

- `src/web/components/terminal/TerminalSession.ts`, `flushInput()`

`flushInput()` removes data from `pendingWriteBuffer`, captures the runtime session ID, and waits asynchronously for `AuthorityGate.authorize('write')`. After authorization it only verifies that the runtime session ID is still current.

A restart can retain the same runtime session ID while replacing the PTY and increasing `terminalRuntimeGeneration`. Consequently:

```text
input for old shell leaves pendingWriteBuffer
  -> authorization is pending
  -> restart replaces the PTY under the same runtime session ID
  -> old authorization resolves
  -> session ID check passes
  -> stale input is written to the new shell
```

The new startup input admission guard does not cover this case because the input already left the buffer before the session entered `awaiting-attach` or `awaiting-replay`.

Recommended correction:

- Add `terminalRuntimeGeneration` to `TerminalWriteInput`.
- Capture the generation and start epoch before asynchronous authorization.
- Reject mismatches client-side after authorization.
- More importantly, validate generation at the server's atomic PTY write boundary.
- Add a test that holds write authorization, restarts, resolves authorization, and proves the old input never reaches the replacement PTY.

### 3. Low/medium: post-open unmeasurable-host error leaks an internal message as an i18n key

Relevant locations:

- `src/web/components/terminal/TerminalSession.ts`, `openPhase()` and `startAsync()` error handling
- `src/web/components/terminal/TerminalSessionView.tsx`, terminal error rendering

If the host is measurable before xterm opens but becomes unmeasurable before `fitNow()`, the code throws:

```text
terminal host became unmeasurable
```

This throw occurs outside the inner `TerminalHostNotMeasurableError` mapping. The outer generic catch stores `err.message` in runtime state, and the UI later passes it to `t(...)` as if it were a traceable localization key.

Recommended correction:

- Normalize every `TerminalHostNotMeasurableError` to `error.terminal-host-not-measurable` at the same boundary.
- Add a test asserting the resulting runtime/UI message key, not only that attach did not occur.

### 4. Test coverage gaps against issue #307

The current tests provide meaningful coverage for fit/refresh ordering, replay admission, hidden startup, fitted attach dimensions, and normal generation-bound resize. The following required or risk-critical cases remain weak or absent:

- No test suspends the final render barrier or replay callback, supersedes the view, and proves the stale term cannot reveal.
- The first-frame visibility test checks the frame after startup has progressed; it does not assert at `term.open()` or the first animation-frame boundary that 80x24 was never visible.
- No test covers a stale resize-triggered takeover crossing a restart.
- The manager generation test rejects a future generation but does not restart and submit a genuinely stale generation.
- The validator test named `requires a valid runtime generation` checks `-1` but not a missing generation field; making the schema optional could leave that test green.
- Font refit, ResizeObserver, reattach, takeover, and recovery do not all have stale-callback/barrier tests.

Recommended additions:

1. Supersede during post-fit barrier and assert no attach/reveal.
2. Supersede during replay/final barrier and assert no reveal.
3. Assert frame visibility is hidden when `term.open()` occurs and through the first render frame.
4. Restart, then submit old-generation resize and takeover operations.
5. Reject resize validation when generation is missing, negative, non-integer, or unsafe.
6. Exercise queued ResizeObserver/font callbacks against a replacement term.

### 5. Low: unrelated development-script change

`scripts/dev.ts` adds `GOBLIN_ELECTRON_USER_DATA_DIR` support. It is not part of issue #307 or the terminal geometry lifecycle fix.

Recommendation: move it to a separate commit/PR to reduce review scope unless it is required by a documented reproduction workflow.

## What is implemented well

The primary presentation pipeline is coherent:

```text
wait for font and measurable host
  -> create hidden xterm with 80x24 internal buffer
  -> wait for initial layout
  -> FitAddon.fit()
  -> refresh the complete fitted buffer
  -> wait through the render barrier
  -> attach/restart PTY using fitted cols/rows
  -> replay the authoritative snapshot/output
  -> wait through the final render barrier
  -> validate the current epoch/term
  -> reveal
```

Other positive points:

- Visibility is owned directly by `TerminalSessionView`, so React metadata timing cannot reveal the internal 80x24 buffer.
- The layout barrier controls ordering and does not introduce a second geometry estimator as authority.
- Initial fit-generated resize events are suppressed until attach/replay has committed.
- Normal resize requests bind start epoch, runtime session ID, and runtime generation.
- The server rejects ordinary resize requests for a mismatched generation.
- Replay and startup input are discarded rather than buffered into a second input authority.
- Epoch/term assertions follow asynchronous render barriers.
- Documentation accurately describes presentation-lifecycle input admission.

## Merge recommendation

Block merge until findings 1 and 2 are fixed with server-side generation checks and regression tests. Finding 3 should preferably be corrected in the same PR because it was introduced by the new `fitNow()` failure path. The broader test gaps should at minimum cover stale reveal, stale takeover, and stale write before approval.

After those corrections, repeat static cross-layer review and run the repository-required validation commands when authorized:

```text
bun run typecheck
bun run test
bun run check:boundaries
```