Skip to content

fix: lock terminal stale restore contract - #659

Merged
Astro-Han merged 1 commit into
devfrom
codex/i648-terminal-pr3
May 16, 2026
Merged

fix: lock terminal stale restore contract#659
Astro-Han merged 1 commit into
devfrom
codex/i648-terminal-pr3

Conversation

@Astro-Han

@Astro-Han Astro-Han commented May 15, 2026

Copy link
Copy Markdown
Owner

Summary

  • Lock DELETE /pty/:id missing-target behavior to 404 NotFoundError.
  • Add backend route coverage for missing PTY remove targets.
  • Add a stale terminal restore E2E that rewrites real workspace terminal storage to legacy pty_old, reloads, and verifies restore uses a durable tab_... without hitting /pty/pty_old.

Why

#648 split terminal identity into durable UI tabs and memory-only runtime PTY handles. PR1 added the storage schema, and PR2 wired the runtime lifecycle. PR3 closes the remaining contract gap: missing delete semantics must be explicit, and stale persisted terminal state must not send old runtime ids back to backend PTY routes.

Related Issue

Closes #648.

Human Review Status

Pending. A human should make the final merge decision after reviewing the final diff and verification evidence.

Review Focus

  • Check that DELETE /pty/:id now matches the existing GET / PUT / connect missing-target contract.
  • Check that frontend durable-tab close behavior still tolerates missing runtime PTYs.
  • Check that the new E2E covers the stale persisted restore path without depending on guessed storage keys.

Risk Notes

  • terminal-reconnect.spec.ts still fails locally at the known Ghostty input path: the command is typed, but the expected token does not render before the reconnect assertion. This is unchanged from PR2 and is not caused by this diff.
  • Manual desktop interaction through Computer Use timed out reading the Electron window. To keep verification bounded, I used isolated desktop startup plus the repo raw CI smoke after building renderer output.
  • No UI layout, copy, terminal rendering, or PTY process implementation is redesigned here.

How To Verify

OpenCode PTY route test: bun --cwd packages/opencode test test/server/pty-routes.test.ts -> 5 pass, 0 fail
App terminal unit tests: bun --cwd packages/app test:unit src/context/terminal.test.ts src/context/terminal-storage.test.ts src/context/terminal-lifecycle.test.ts -> 1112 pass, 0 fail
App typecheck: bun --cwd packages/app typecheck -> passed
OpenCode typecheck: bun --cwd packages/opencode typecheck -> passed
Diff check: git diff --check -> passed
Terminal stale restore E2E: bun --cwd packages/app test:e2e:local -- e2e/terminal/terminal-stale-restore.spec.ts -> 1 passed
Terminal init E2E: bun --cwd packages/app test:e2e:local -- e2e/terminal/terminal-init.spec.ts -> 1 passed
Terminal reconnect E2E: bun --cwd packages/app test:e2e:local -- e2e/terminal/terminal-reconnect.spec.ts -> failed at known Ghostty input token render step before reconnect assertion
Desktop build: bun --cwd packages/desktop-electron build -> passed
Desktop raw smoke: bun packages/desktop-electron/scripts/ci-smoke.ts raw dev -> passed

Screenshots or Recordings

N/A. This PR changes runtime recovery contracts and regression coverage, not visible UI layout or copy.

Checklist

  • Human review status is stated above as pending, approved, or not required
  • I linked the related issue, or stated why there is no issue
  • This PR has type, primary area, and priority labels, or I requested maintainer labeling
  • I described the review focus and any meaningful risks
  • I listed the relevant verification steps and the key result for each
  • I did not introduce unrelated refactors, dependencies, generated files, or file changes beyond the stated scope
  • I manually checked visible UI or copy changes when needed, with screenshots or recordings
  • I considered macOS and Windows impact for platform, packaging, updater, signing, paths, shell, or permissions changes
  • I called out docs, release notes, dependencies, permissions, credentials, deletion behavior, generated content, or local file changes when relevant
  • I reviewed the final diff for unrelated changes and suspicious dependency changes
  • I am targeting dev, and my PR title and commit messages use Conventional Commits in English

Summary by CodeRabbit

Release Notes

  • Bug Fixes

    • Enhanced terminal session deletion to validate session existence before removal, providing clearer error messages
  • New Features

    • Terminal restoration now creates fresh runtime sessions when resuming previously persisted terminal states
  • Tests

    • Added comprehensive test coverage for terminal state restoration workflows and error handling in terminal session operations

Review Change Stack

@coderabbitai

coderabbitai Bot commented May 15, 2026

Copy link
Copy Markdown
Contributor

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: c7771958-5625-489a-a1dc-88ce87b8c56b

📥 Commits

Reviewing files that changed from the base of the PR and between c279ef5 and d616eae.

📒 Files selected for processing (3)
  • packages/app/e2e/terminal/terminal-stale-restore.spec.ts
  • packages/opencode/src/server/instance/pty.ts
  • packages/opencode/test/server/pty-routes.test.ts

📝 Walkthrough

Walkthrough

Backend validation enforces that missing PTY resources return 404 NotFoundError on delete. E2e test validates that restoring legacy persisted terminal state with stale runtime PTY IDs creates fresh PTYs, prevents old resource requests, and migrates persistence to version 2 without embedding runtime handles.

Changes

Stale Terminal Restore and Missing-PTY Contract

Layer / File(s) Summary
Missing-PTY backend contract
packages/opencode/src/server/instance/pty.ts
DELETE :ptyID handler now loads the session first, raises NotFoundError("Session not found") if missing, then removes it.
Backend test for missing PTY
packages/opencode/test/server/pty-routes.test.ts
Test verifies that DELETE /pty/<non-existent> returns 404 status with NotFoundError name in JSON body.
E2E test for stale terminal restore
packages/app/e2e/terminal/terminal-stale-restore.spec.ts
Injects legacy persisted terminal state into localStorage, reloads the page, opens the terminal, and asserts: new PTY tab ID does not match the old stale ID, no network traffic targets the old /pty/pty_old resource, and persisted state is migrated to v2 without pty_old or runtime runtimePty references.

🎯 2 (Simple) | ⏱️ ~12 minutes


bug


🐰 Stale terminal IDs shuffle off to rest,
Fresh PTYs bloom when sessions are refreshed,
Migrations v2 banish ghosts of the past,
No 404 loops to forever last,
A rabbit hops through clean state at last! 🌿

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title 'fix: lock terminal stale restore contract' clearly describes the main change: enforcing backend and frontend contract behavior for terminal state restoration.
Description check ✅ Passed The PR description comprehensively covers all template sections including summary, rationale, related issue, review focus, risk notes, verification steps with results, and a completed checklist.
Linked Issues check ✅ Passed The PR fully addresses all coding requirements from #648: implements explicit DELETE /pty/:id missing-target semantics [pty.ts], adds server route coverage for missing PTY removal [pty-routes.test.ts], and provides regression E2E coverage for stale persisted restore [terminal-stale-restore.spec.ts].
Out of Scope Changes check ✅ Passed All three changed files are directly aligned with #648 objectives: contract enforcement (pty.ts), server test coverage (pty-routes.test.ts), and regression E2E (terminal-stale-restore.spec.ts) for terminal stale restore. No unrelated refactors or out-of-scope changes detected.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch codex/i648-terminal-pr3

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@Astro-Han Astro-Han added P2 Medium priority app Application behavior and product flows tech-debt Supplemental cleanup, maintainability, architecture, test, or quality debt context labels May 15, 2026
@github-actions github-actions Bot added the harness Model harness, prompts, tool descriptions, and session mechanics label May 15, 2026

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested priority: P2 (includes non-doc, non-test paths outside the low-risk bucket).

P1/P0 are reserved for maintainer confirmation. Please relabel manually if this is a release blocker, security issue, data-loss risk, or updater/runtime failure.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces an E2E test to verify that restoring legacy terminal state correctly initializes a fresh runtime PTY. It also updates the PTY deletion route to explicitly check for existence and return a 404 error, supported by a new unit test. Feedback suggests optimizing the deletion logic by removing the redundant existence check before removal and using a more generic error message to avoid information leakage.

Comment thread packages/opencode/src/server/instance/pty.ts
@Astro-Han

Copy link
Copy Markdown
Owner Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented May 15, 2026

Copy link
Copy Markdown
Contributor
✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@Astro-Han
Astro-Han merged commit 57cb802 into dev May 16, 2026
28 checks passed
@Astro-Han
Astro-Han deleted the codex/i648-terminal-pr3 branch May 16, 2026 00:12
Astro-Han added a commit that referenced this pull request May 17, 2026
Root cause / goal:
- The latest public stable release v2026.5.15 predates the stale persisted session and terminal recovery fixes from #649/#659.
- Installed users can still reopen stale pinned sessions or terminal PTY state, causing repeated reconnect attempts and noticeable desktop jank.

Change boundary:
- Bump the desktop package version to 2026.5.17 so the existing fixes can be built and published as a hotfix release.
- No runtime behavior changes are introduced by this release PR itself.

Verification:
- Local app unit suite: 1146 pass / 0 fail.
- OpenCode PTY route tests: 5 pass / 0 fail.
- Terminal stale restore E2E: 1 passed.
- Web smoke: 22 passed / 1 skipped.
- Electron build, runtime import guard, raw smoke, problem report smoke, macOS arm64 dir packaging, and packaged smoke all passed locally.
- PR CI, CodeQL, desktop smoke, E2E artifacts, dependency review, lint, typecheck, and unit shards passed.

Release follow-up:
- Run the production desktop release workflows for macOS arm64/x64 and Windows x64.
- Publish v2026.5.17 only after all expected assets and updater metadata are present.

Refs #647.
Refs #648.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

app Application behavior and product flows harness Model harness, prompts, tool descriptions, and session mechanics P2 Medium priority tech-debt Supplemental cleanup, maintainability, architecture, test, or quality debt context

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Task] Split durable terminal tabs from runtime PTY handles

1 participant