Skip to content

Keep mirrored remote terminal metadata - #18602

Closed
Cubatica wants to merge 1 commit into
stablyai:mainfrom
Cub-HQ:fix/remote-inventory-connected-hosts
Closed

Cubatica wants to merge 1 commit into
stablyai:mainfrom
Cub-HQ:fix/remote-inventory-connected-hosts

Conversation

@Cubatica

@Cubatica Cubatica commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

ELI5

When a paired remote Orca tab is mirrored locally, the local runtime can know that a terminal exists without having the remote repo registered locally. In that case terminal list should still show the useful tab title, worktree path, and restored preview instead of a blank row.

What Changed

  • Added fallback metadata for terminal summaries from the PTY record and mirrored mobile-session terminal snapshot.
  • Falls back to the parsed worktree path embedded in the worktree id when the local profile has no matching repo/worktree record.
  • Added a regression test for paired-runtime terminals where the local profile lacks the remote repo.

Why

This addresses a paired M1/Intel failure where direct remote inventory returned full terminal metadata, but the default local/federated terminal list showed remote rows with blank title/path/branch/preview. The fix keeps the existing host-scope semantics intact: mirrored runtime rows still do not claim authoritative host inventory coverage.

Linked Issue

Fixes #18595

Visual Proof

N/A. This is a runtime/CLI metadata fix covered by a regression test; no visual asset was changed.

Testing

  • I manually tested these changes locally
  • Automated tests added/updated, or explained why not below

Commands run on macOS:

npm exec --yes pnpm@12.0.0 -- vitest run src/main/runtime/terminal-list-execution-host-scope.test.ts
npm exec --yes pnpm@12.0.0 -- run typecheck:node
npm exec --yes pnpm@12.0.0 -- run check:code-quality:changed
git diff --check

Note: the global pnpm shim on this machine is currently corrupted for the pinned pnpm@12.0.0 path, so I used npm exec --yes pnpm@12.0.0 for the local validation commands.

AI Disclosure

AI assistance was used to diagnose the issue, implement the patch, and draft this PR.

Review

Self-reviewed for scope, host-scope truthfulness, and regression coverage.

Agent skill upstream boundary

  • Not applicable, or this change follows docs/reference/agent-skill-sharing-upstream-boundary.md and copies or mechanically translates no upstream skill-installer source, tests, fixtures, registry entries, path tables, comments, or documentation.

Notes

Cross-platform impact should be low: the path fallback only parses the existing runtime worktree id format and is used only when no resolved worktree metadata exists.

Checklist

  • This PR is small and focused
  • I explained what changed and why (including ELI5)
  • Before/after screenshots or videos attached for UI changes, or N/A with reason
  • Self-reviewed for correctness, security, and performance
  • Cross-platform, SSH/remote, and path/shortcut impact considered (or N/A)
  • pnpm lint, pnpm typecheck, pnpm test, and pnpm build pass (or CI will cover; local preferred)

@coderabbitai

coderabbitai Bot commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Team

Run ID: 8f90bd93-51e8-4529-a491-107c6ec94265

📥 Commits

Reviewing files that changed from the base of the PR and between 2ee507d and f56dfcd.

📒 Files selected for processing (2)
  • src/main/runtime/orca-runtime-write-orchestration-pointer-pty.ts
  • src/main/runtime/terminal-list-execution-host-scope.test.ts

Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review.


📝 Walkthrough

Walkthrough

Runtime terminal summaries now derive titles, worktree paths, output timestamps, and previews from leaf, PTY, and mobile terminal-tab data. Runtime status fields use these derived values. A regression test covers restored mirrored terminals when the local repository is absent. The test verifies local-only host scope and preservation of the runtime host ID, worktree path, restored title, and output preview.

Merge Risk: ⚪ Minimal · up to f56df

Mirrored terminals without locally registered repositories now retain titles, worktree paths, and output previews while continuing to report non-authoritative host scope correctly. No current merge-readiness risk remains.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly describes the main change: preserving metadata for mirrored remote terminals.
Description check ✅ Passed The description follows the required template and explains the change, reason, linked issue, testing, scope, and compatibility impact. It also documents that the full lint, test, and build commands we…
Linked Issues check ✅ Passed The changes satisfy the relevant terminal-metadata objective for issue #18595. They preserve mirrored terminal titles, worktree paths, previews, and timestamps when local repository metadata is unavai…
Out of Scope Changes check ✅ Passed The changes are limited to terminal summary metadata fallbacks and a focused regression test. They directly support the linked issue and do not introduce unrelated behavior.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 2…

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.

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

ℹ️ No critical issues — one minor suggestion inline.

Reviewed changes

  • Metadata fallback in buildTerminalSummary — title now falls back through the PTY record (getLatestPtyTitle / controllerTitle) and the mirrored mobile-session terminal snapshot, worktree path falls back to the parsed worktree id, and lastOutputAt / preview blend leaf and PTY values.
  • Regression test — keeps mirrored paired-runtime terminal metadata when the local profile lacks that repo covers the blank-title/path/preview failure with exact value assertions, so it fails on regression rather than passing on a loose shape.

Pullfrog  | Fix all ➔ | Fix 👍s ➔ | View workflow run | Using DeepSeek Pro (free via Pullfrog for OSS) | 𝕏

(pty ? (getLatestPtyTitle(pty) ?? pty.controllerTitle) : null) ??
mobileTerminalTab?.title ??
null
const parsedWorktree = parseRuntimeWorktreeId(leaf.worktreeId)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

parseRuntimeWorktreeId preserves the folder-workspace instance suffix (it uses splitWorktreeId, not splitWorktreeIdForFilesystem). A mirrored remote terminal whose worktree id embeds ::workspace:<uuid> would surface /folder::workspace:<uuid> as its display path instead of /folder. Use splitWorktreeIdForFilesystem (or getWorktreePathBasenameFromId) for the display fallback so it matches how buildResolvedWorktreeFromId derives real paths.

@nwparker

nwparker commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

Closing after an adversarial maintainer review. The desired outcome is reasonable, but this patch does not establish or safely fix the production failure.

Blocking findings:

  1. The regression test fabricates a topology that the real paired-runtime path does not publish. Real mirrored terminals are web-terminal-* tabs, and isWebOnlyMirroredTerminalTab deliberately excludes remote-only mirrors from both the local runtime graph and the locally republished mobile snapshot. The test instead directly inserts mobileSessionTabsByWorktree and syncs a normal tab-remote leaf with the same remote PTY. That bypasses the production ownership/echo boundary.

  2. The patch changes only buildTerminalSummary, which summarizes graph leaves. A real web-mirrored terminal is excluded from that graph. If a retained row is emitted from ptysById, it goes through buildPtyTerminalSummary, which this PR leaves unchanged, including the blank path behavior.

  3. The title fallback matches only worktree/tab/leaf. It does not require the snapshot PTY id or incarnationId to match the current leaf. After a same-surface rebind, it can assign the previous process title to the replacement. That title also feeds resolvePaneAgentIdentityField, so this is not merely cosmetic stale text.

  4. The path fallback uses parseRuntimeWorktreeId, which preserves the synthetic ::workspace:<uuid> suffix. Folder workspaces would expose an identifier as part of the displayed filesystem path. The existing filesystem boundary uses splitWorktreeIdForFilesystem for this reason.

  5. The preview assertion does not pin the new fallback. The test seeds the PTY before syncWindowGraph; graph sync already copies the PTY tail into the leaf, so the assertion passes without leaf.preview || pty.preview.

  6. The reported blank branch is not addressed, the test does not exercise reconnect/resync or the actual host-session mirror, and this head has only community bookkeeping/review checks rather than the full required CI.

#18619 fixed the root cause and closed #18595. If a metadata-only defect remains after that fix, it should be reproduced through the real host-session mirror boundary with captured terminal.list and session.tabs state. A narrow filesystem-path fallback may still be useful independently, but it should cover both leaf and PTY summaries, use splitWorktreeIdForFilesystem, and have folder-workspace/Windows tests. None of this commit is safe to cherry-pick as-is.

@nwparker nwparker closed this Sep 4, 2026
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.

M1 remote inventory omits connected Intel runtime, blanking remote tabs after 1.4.197

2 participants