Sign up
View stargazersSign up

fix(server): prevent legacy terminal history collisions across threads#24574

open9
main
takeover/pr-18671
opened3d agobyJJulius Marminge
2 files changed
View on GitHubTwo-way sync enabledPR last imported 3d ago

Legacy history lookup could resolve another thread’s current history file, exposing its scrollback or deleting it. The server now rejects legacy filenames in the case-insensitive terminal_ namespace across migration and both deletion paths.

Original description by @Batuhan1999

Problem

A legacy terminal history filename can collide with another thread's current history file. Opening or deleting a terminal for a client-supplied ID such as terminal_YQ can read or remove thread a's history.

Change

Reserve the terminal_ namespace, ignoring letter case, in one legacy-name resolver. History migration, individual terminal deletion and whole-thread deletion all use that resolver. Current history handling and ordinary legacy migration remain intact.

Scope and approval

Preserves @Batuhan1999's original contribution in #18671, originally separated from #18669. Main snapshot a3ca1a57d6 is integrated with contributor ancestry retained. This changes the server's history filename handling; clients, contracts and providers are unchanged.

Verification

At 9f70943e77, based on a3ca1a57d6:

  • All 103 terminal-manager tests pass, along with server typecheck and focused lint/format checks.
  • Reduced the original two cases to one public Manager lifecycle test. It verifies empty history on the colliding thread and preservation of the other thread's file after both individual and whole-thread deletion.
  • Four temporary guard-removal probes independently reproduced the read/migration, individual-delete and whole-thread-delete failures. Production code was restored byte-for-byte.
  • Fresh full cumulative Opus 5.5 and GPT-6.1 Sol reviews found no code defects. Main's bounded history, inherited-history neutralization, tracing, shell startup and closed-terminal behavior are retained.

Filesystem behavior was exercised on Linux. The mixed-case protection for case-insensitive macOS/Windows filesystems was reviewed from source but was not run on those filesystems. No separate browser/device session was needed for this server filesystem change.

Implementation and tests: Claude Opus 5.5 and Claude Fable 5.1 through T3 Code. Independent review: Claude Opus 5.5 and GPT-6.1 Sol, standard service tiers.

Takeover

Credits @Batuhan1999’s original contribution in #18671; contributor ancestry is retained on this replacement branch. Adapted the guard to main’s bounded-filename API and shared it with whole-thread deletion. Replaced the original regression with lowercase and mixed-case lifecycle cases. Reserved-prefix legacy files are intentionally ignored because they can overlap encoded or hashed current history filenames.

Evidence

Fixture matrix · Runtime capture report · Public drivers, transcripts and logs

Fixture rowsBeforeAfterProof
R1 read/migrateActor reads owner scrollback; owner loses historyActor empty; owner history retainedReal server RPC and service transcript
R2 individual deleteOwner history removedOwner history retainedReal server RPC and service transcript
R3 whole-thread deleteOwner history removedOwner history retainedReal server RPC and service transcript
M1–M3 mixed-caseAlias surrogate read/removedSurrogate retainedLinux service transcript; native aliasing unverified
S1–S3 sanitized prefixOwner history read/removedOwner history retainedService transcript
H1–H3 hashed nameOwner history read/removedOwner history retainedService transcript
L1–L3 ordinary legacyMigrates/deletes intended historySameCompatibility controls
C1–C3 current/default/namedLoads/deletes intended filesSameCompatibility controls
P1–P3 reserved ID own-currentOwn history works; deletion harms ownerOwn history works; owner preservedService transcript
C6 bounded filenames (9 cases)Encoded/hashed default and named workSameCompatibility controls
T1 orderingFresh seed per independent actionSameSequential calls; persisted-file snapshots

Captures: before 909e4c9482, after dc98776633. Linux x64 / Node 26.12.1; real files, inert-PTY service scenarios plus real node-pty and WebSocket RPC. All seeds are synthetic, in fresh homes under /tmp/ev-18671; no live userdata. No render path changed, so UI captures and pixel diffs do not apply. What else changed: no behavior beyond guarded legacy resolution and deletion; ordinary/current-history controls agree.

Verification log
  • 45/45 focused Manager cases pass after final main merge; removing the guard fails both collision cases, removing case folding fails mixed-case only. Main had 43 existing cases; the original PR added one; replacement adds two parameterized cases.
  • Focused lint/format and server typecheck pass; one existing lint warning remains in unchanged code.
  • One independent full-diff review round: GPT-6.1 Sol MERGE and Claude Opus 5.5 MERGE; no blocking findings. Existing ordinary legacy migration coverage confirmed.
  • Complexity: linear ID sanitization/case folding; directory filter O(entries × ID length). Approximately 3,000-token malformed probes remain sub-millisecond; no new regex/parser.
  • Final head: 4f9c30a87f, merged main 3cc31ed27c. Both terminal files are byte-identical to the captured revision. Final-head proof audit: SUFFICIENT, including final-head RPC rerun.
  • CI: pending publication; no merge performed.
  • Unverified surfaces: native case-insensitive macOS/Windows filesystem execution; Linux mixed-case surrogate verifies guard semantics. Only server paths changed; web, desktop and mobile captures do not apply.
  • Implementation/tests: Claude Opus 5.5 via T3 Code. Reviews: GPT-6.1 Sol and Claude Opus 5.5. Evidence/proof audit/PR lead: GPT-6.1 Sol through Codex in T3 Code.

Checks

20 successful · 10 skipped · 1 neutral
All checks passed
4f9c30a

Added 9 commits

  • Merge remote-tracking branch 'origin/main' into takeover/1867131f6b87
  • Merge remote-tracking branch 'origin/main' into takeover/pr-186714f9c30a
  • fix(server): legacy terminal history guard ignores letter casing54c5f2a
  • fix(server): terminal history no longer crosses threads via legacy names89dee45
  • Merge commit 'a3ca1a57d65093c3f19d83f2c2b6d63663a380e1' into takeover/186719f70943
  • fix(server): legacy terminal history lookup skips names owned by current threadsa9d0736
  • Merge commit 'e704190e8ce0d15f85e42bc844baad525b2110c1' into takeover/18671abe222f
  • Merge remote-tracking branch 'origin/main' into takeover/pr-18671bdf9f98
  • test(server): legacy terminal history name never resolves to another thread's filedc98776
MmacroscopeappBot3d ago
Synced from GitHub

Approvability

Verdict: Needs Human Approval

Macroscope's review found this PR requires human approval — This is a focused fix for legacy terminal-history filename collisions, with regression coverage for reads and both deletion paths. Because it changes cross-thread access and deletion of terminal scrollback, the sensitive-data impact warrants human review.

You can add or adjust custom eligibility rules. Learn more.

Ggithub-actionsBot3d ago
Synced from GitHub

Thread transfer impact

✅ Thread transfer remains within every enforced ceiling.

ProviderMetricMain baselineThis PRImpactPR ceiling
CodexTotal thread wire5.0 KiB5.0 KiB−7 B (−0.1%)6.8 KiB✅
CodexThread snapshot wire3.8 KiB3.8 KiB0 B (0.0%)4.9 KiB✅
CodexLive turn WebSocket wire1.2 KiB1.2 KiB−7 B (−0.6%)2.0 KiB✅
CodexLive turn WebSocket decoded20.9 KiB20.9 KiB0 B (0.0%)29.3 KiB✅
CodexLive turn messages220 (0.0%)8✅
ClaudeTotal thread wire5.0 KiB5.0 KiB+24 B (+0.5%)6.8 KiB✅
ClaudeThread snapshot wire3.8 KiB3.8 KiB0 B (0.0%)4.9 KiB✅
ClaudeLive turn WebSocket wire1.2 KiB1.2 KiB+24 B (+2.0%)2.0 KiB✅
ClaudeLive turn WebSocket decoded21.2 KiB21.2 KiB+41 B (+0.2%)29.3 KiB✅
ClaudeLive turn messages12+1 (+100.0%)8✅

Baseline: 3cc31ed · PR result: 4f9c30a · Source CI: success

Scenario and decoded snapshot size

10 historical turns, 5 command tools per turn, 878.9 KiB retained MCP result per historical turn, and a 1.05 MiB retained result in the measured turn.

  • Codex decoded thread snapshot: 108.5 KiB
  • Claude decoded thread snapshot: 108.8 KiB

Updated in place by a trusted workflow. PR artifacts are strictly validated and never executed.

CcoderabbitaiBot3d ago
Synced from GitHub

Review in Change Stack →

No actionable comments were generated in the recent review.

External image Recent review info
External image Run configuration
External image Commits

Reviewing files that changed from the base of the PR and between 3cc31ed27c8956b36cee35a50509fb7c6869592b and 4f9c30a87fd3a3114efc66b13a3cac819465f275.

External image Files selected for processing: 2
  • apps/server/src/terminal/Manager.test.ts
  • apps/server/src/terminal/Manager.ts

Limit details: You’ve used all 10 included reviews currently available.

Walkthrough

Legacy terminal-history handling now excludes eligible-looking filenames when the sanitized thread ID begins with terminal_, regardless of case. Tests verify that opening and deleting matching threads leave those files unchanged.

Changes: Legacy history path guard

Layer / File(s)Summary
Legacy history eligibility and deletion
apps/server/src/terminal/Manager.ts, apps/server/src/terminal/Manager.test.ts		The legacy-path check excludes sanitized thread IDs beginning with terminal_ without regard to case. Bulk deletion uses the same check. Tests cover lowercase and mixed-case filenames and confirm their contents remain unchanged.

Priority: External image Normal

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix

Possibly related PRs

  • pingdotgg/t3code#18671: Adds a guarded legacy-name resolver for reads and deletion; this PR applies the guard to the bulk-deletion path.

Suggested labels: bugfix, size:XS

Suggested reviewers: batuhan1999

Merge Risk: External image Minimal · up to 4f9c3

The change prevents legacy history collisions without identifying a remaining merge-blocking risk.

Pre-merge checks: Passed 4
Check nameStatusExplanation
Title checkPassedThe title clearly and concisely describes the main server-side fix: preventing legacy terminal history collisions across threads.
Description checkPassedThe description includes all required sections: Problem, Change, Scope and approval, and Verification. It provides reproduction context, implementation details, approval history, test results, evidenc…
Linked Issues checkPassedCheck skipped because no linked issues were found for this pull request.
Out of Scope Changes checkPassedCheck skipped because no linked issues were found for this pull request.
Full details: Description check

Explanation

The description includes all required sections: Problem, Change, Scope and approval, and Verification. It provides reproduction context, implementation details, approval history, test results, evidence, and known unverified platforms.

Finishing Touches
📝 Generate docstrings
  • Commit to this branch
  • Create a new PR
🧪 Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Usage-based review receipt

Note

This review was completed with usage-based billing: files reviewed beyond your plan's included limits are billed at $0.25/file. View usage-based billing.

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