Repository navigation
feat(ssh): batch process evidence in PTY inventory - #17525
Conversation
Greptile SummaryThe PR adds batched POSIX process-table correlation and carries validated foreground-process evidence from SSH relay PTY inventory into the renderer.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains.
|
| Filename | Overview |
|---|---|
| src/shared/process-table-snapshot.ts | Adds strict process-table parsing and indexing; the prior rejection of valid zero PGIDs is corrected with matching coverage. |
| src/main/providers/agent-foreground-process-batch.ts | Correlates PTY roots and foreground process groups from a shared process-table index. |
| src/relay/pty-handler.ts | Captures one POSIX process table per inventory request and attaches additive foreground evidence to PTY summaries. |
| src/main/providers/pty-process-list-admission.ts | Validates, accounts for, and clones optional foreground-process evidence at the provider boundary. |
| src/renderer/src/components/terminal-pane/agent-completion-process-monitor.ts | Integrates admitted host foreground evidence into terminal agent process monitoring. |
Sequence Diagram
sequenceDiagram
participant Renderer
participant Main as Main Process
participant Relay as SSH Relay
participant PS as POSIX Process Table
Renderer->>Main: Request PTY inventory
Main->>Relay: pty.listProcesses
Relay->>PS: Capture one host-wide snapshot
PS-->>Relay: pid/ppid/pgid/tpgid/stat/command rows
Relay->>Relay: Strictly parse and build process index
Relay->>Relay: Correlate each PTY with foreground group
Relay-->>Main: Inventory with optional foreground evidence
Main->>Main: Validate and clone evidence
Main-->>Renderer: Admitted PTY inventory
Renderer->>Renderer: Update process monitor
Reviews (3): Last reviewed commit: "fix(ssh): preserve process evidence poll..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
📝 WalkthroughWalkthroughThe change adds strict process-table parsing with process-group indexes and snapshot APIs. It adds batched foreground-agent resolution with explicit unavailable reasons and evidence conversion. It defines validated foreground-process evidence and preserves it during PTY process admission. POSIX PTY listings now capture one process-table snapshot, resolve entries in a batch, and attach evidence metadata. The renderer now controls process polling by host and recent pane activity. Merge Risk: 🔵 Low · up to The PR adds foreground-process evidence to PTY inventory, but a correlation failure can currently lose the existing process-resolution fallback, and callers reaching the relay route may receive recognized agent-activity metadata without an explicit authorization check. The change is mergeable with owner awareness and follow-up on these bounded correctness and disclosure risks. 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Full details: Description checkExplanation The description includes a useful summary and testing section, but it omits most required template sections, including ELI5, What Changed, Why, Linked Issue, Visual Proof, AI Disclosure, Review, Notes, and Checklist. Resolution Complete the required template sections. Add a linked issue using
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/shared/process-table-snapshot.ts (1)
53-55: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the unreachable fallbacks and the cast in
parseProcessTableRows.Groups 5 and 6 are mandatory in the pattern, so
match[5]andmatch[6]are always defined on a successful match.?? match[3]and?? match[4]never run. Theas ProcessTableRowcast is what hides this from the type checker.♻️ Proposed simplification
- stat: match[5] ?? match[3], - command: match[6] ?? match[4] - } as ProcessTableRow) + stat: match[5], + command: match[6] + })src/main/providers/agent-foreground-process-batch.ts (1)
129-134: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOne lookup-counting helper is defined twice.
lookupIndexand the exportedlookupProcessTableIndexhave identical bodies, so the two copies can drift on how lookups are counted.
src/main/providers/agent-foreground-process-batch.ts#L129-L134: deletelookupIndexand importlookupProcessTableIndexfrom../../shared/process-table-snapshot.src/shared/process-table-snapshot.ts#L163-L172: keep this helper as the single definition. If no other caller needs it exported, also drop the unusedparseProcessTableRowsStrictalias at Line 111 in the same pass.src/relay/pty-shell-utils.ts (1)
244-244: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winAccept
readonly ProcessTableRow[]to avoid a per-pane copy of the whole process table.The mutable parameter type forces the caller to clone.
src/relay/pty-handler.ts:2290calls this function with[...evidenceRows]inside the per-entry loop, so a listing with 200 panes copies the entire host process table 200 times. The function only readsrows.♻️ Proposed signature change
export function getForegroundProcessNameFromProcessTable( - rows: ProcessTableRow[], + rows: readonly ProcessTableRow[], pid: number, fallbackProcess?: string | null ): string | null {
collectDescendantsneeds the same widening:function collectDescendants( - rows: ProcessTableRow[], + rows: readonly ProcessTableRow[], rootPid: number ): (ProcessTableRow & { depth: number })[] {Then drop the spread at the call site in
src/relay/pty-handler.ts:- [...evidenceRows], + evidenceRows,
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: d151269e-398d-4efa-9cc9-5dbbc55f299a
📒 Files selected for processing (10)
src/main/providers/agent-foreground-process-batch.test.tssrc/main/providers/agent-foreground-process-batch.tssrc/main/providers/agent-foreground-process.tssrc/main/providers/pty-process-info.tssrc/main/providers/pty-process-list-admission.test.tssrc/main/providers/pty-process-list-admission.tssrc/relay/pty-handler.tssrc/relay/pty-shell-utils.tssrc/shared/foreground-process-evidence.tssrc/shared/process-table-snapshot.ts
Included review availability: Your plan provides up to 10 included reviews per hour; 3 remain after this review.
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
parseStrictProcessTableRows rejects pgid <= 0, but every real Linux ps -axo … pgid=,tpgid=… capture contains kernel threads with pgid=0, so the strict evidence snapshot throws on every scan. The feature never produces live evidence in production — see the inline comment for details.
Reviewed changes
- Strict evidence capture —
PS_ARGSgainspgid/tpgidcolumns and a newparseStrictProcessTableRowstreats any malformed row as an unreadable capture, backed by a single-passbuildProcessTableIndex. - Batched correlation —
resolveAgentForegroundProcessesBatch/…FromIndexwalk one host-wide index once and resolve foreground agents for all managed PTYs viapgid === tpgidassociation. - Relay inventory wiring —
PtyHandler.listProcessescaptures one strict snapshot per listing (POSIX only) and attaches optionalforegroundProcessEvidenceto each summary, while Windows keeps the legacy title/liveness path. - Admission validation —
PtyProcessListAdmissionvalidates, clones, and byte-budgets the newforegroundProcessEvidencefield. - Refactor —
getForegroundProcessNameFromProcessTableis extracted from the relay shell utils for reuse against the shared snapshot.
DeepSeek Pro (free via Pullfrog for OSS) ｜ 𝕏
| const children = childrenByPpid.get(row.ppid) ?? [] | ||
| children.push(row) | ||
| childrenByPpid.set(row.ppid, children) | ||
| if (row.pgid !== undefined) { |
There was a problem hiding this comment.
byPgid and byTpgid are built on every capture but never read — resolveAgentForegroundProcessesFromIndex filters rowsByOwner (descendants) by row.pgid === root.tpgid instead. That also misses a foreground process that was reparented away from the shell (parent exited while the PTY survives), which byTpgid would find. Either consume byTpgid for the foreground-group association or drop both unused maps.
| { | ||
| authorityGeneration: this.ptyIdMintEpoch, | ||
| observationEpoch: evidenceEpoch, | ||
| capturedAgeMs: 0 |
There was a problem hiding this comment.
capturedAgeMs is hardcoded to 0, but the field's contract ("receivers rebase this onto their monotonic clock") implies a real measured age. The strict reader already tracks capturedAtMs internally — expose it and populate this field, or drop capturedAgeMs until it is actually measured.
|
the model provider had a temporary upstream failure. The provider refused or dropped the request mid-run — nothing in your repo or your credentials is at fault. Re-trigger Pullfrog; if it keeps happening, check the provider's status page.
|
PR #17525 Electron visual proof — PTY/process inventoryPR: #17525 No source files were modified. IdentityLaunched this exact worktree via
{
"name": "Orca: brennanb2025/r1-build",
"isDev": true,
"devLabel": "r1-build",
"devBranch": "brennanb2025/r1-build",
"devWorktreeName": "r1-build",
"devRepoRoot": "/Users/brennanbenson/orca/workspaces/orca/r1-build",
"dockBadgeLabel": null
}Launch log: macOS Dock tile names included Scenario exercisedIsolated fresh profile (no imported user sessions).
SSH/relay inventory was not reachable in this isolated profile (no SSH target configured). That is the PR’s stated remaining live-validation gap. Visible evidence1. Empty inventory (0 sessions)
2. Terminal pane with live PTY + probe command
3. Resource Manager inventory popover
Accessibility snapshot of the session row:
The Resource Manager row label is the tab title Terminal 1. It does not show foreground process name ( Local screenshots also live at:
Backing statePTY id:
[{
"id": "81971d5c-d099-4fc6-9562-87567ff26a05::/Users/brennanbenson/orca/orca@@188d3ddc",
"cwd": "/Users/brennanbenson/orca/orca",
"title": "shell",
"agentOwnership": "absent"
}]
{
"sessionId": "81971d5c-d099-4fc6-9562-87567ff26a05::/Users/brennanbenson/orca/orca@@188d3ddc",
"incarnationId": "2abea51e-73b8-4916-8840-b82f9cc41f18",
"state": "running",
"shellState": "unsupported",
"isAlive": true,
"terminalHandle": "term_b420ee13-4b9f-42f5-80c7-f2f70f433dca",
"wslDistro": null,
"pid": 10999,
"cols": 135,
"rows": 60,
"agentSessionOwners": [],
"protocolVersion": 36
}
{ "foregroundProcess": "login", "hasChildProcesses": true }Host process evidence (outside the app):
PTY buffer snapshot (
Local inspect reporting LogsLaunch log
Renderer CDP console: 0 errors, 2 warnings (no inventory-related failures observed). Gaps
This is not a merge-confidence claim for the SSH inventory change. It is visual proof that this worktree’s app boots, that local PTY session inventory is reachable in Resource Manager, and that foreground-process evidence is not shown in the current renderer. |
Review status — land after fixes (PASS WITH RISK)Reviewed the PR through the final pushed commit Fixes applied
Verification
Remaining validation gapLive SSH relay No remaining proven P0/P1/P2 code issues were found. The worktree is clean, and the fixes are pushed to the PR branch. |
There was a problem hiding this comment.
ℹ️ No critical issues — the strict-parser blocker from the prior review is resolved, and the delta is clean. One informational note on renderer wiring below.
Reviewed changes
- Strict parser accepts kernel rows —
parseStrictProcessTableRowsnow rejects only domain-invalid values (pid <= 0,ppid < 0,pgid < 0,tpgid < -1), accepting thepgid=0/tpgid=-1kernel-thread rows that broke every real scan, pinned by a real-capture fixture. - Batched title reuse — the per-PTY title now reads the batched evidence result instead of rebuilding a per-pane descendant walk, removing the O(panes × table) copy the prior review flagged.
- Wrapper-foreground alignment — the batch path now mirrors the legacy inspection filter (
isExpectedAgentProcess(getFirstCommandToken(...))) and hands all descendants toresolveOuterWrapperForegroundProcess, so the outer-wrapper walk can see non-foreground ancestors. - Push-driven no-evidence polling option — a
shouldPollNoEvidenceProcessCadencecoordinator option, a null-initlastPaneActivityAt, and a hot-window re-arm were added with three focused tests.
ℹ️ The new no-evidence polling option is tested but not wired
shouldPollNoEvidenceProcessCadence is added to the coordinator API and exercised by three new tests, but no production caller sets it: terminal-keydown-fit.ts was reverted in bf758b9 to preserve existing polling semantics, and its comment now says the no-evidence cadence stays on "until inventory evidence is consumed by this renderer; mixed-version relays may omit the optional field." The result is that none of the new renderer coordinator behavior is reachable in production today — the new option, the hot-window re-arm branch, and the lastPaneActivityAt null-refactor are all inert (the latter is behavior-neutral). This looks consistent with the PR's stated "does not open renderer gates" scope, but it means the delta ships speculative plumbing plus tests for a path nothing activates; if the renderer evidence-consumption lands as a follow-up, consider moving the option and its tests with it.
Technical details
# No-evidence polling option is dead until a renderer consumes evidence
## Affected sites
- src/renderer/src/components/terminal-pane/agent-completion-coordinator-types.ts:46 — `shouldPollNoEvidenceProcessCadence` option.
- src/renderer/src/components/terminal-pane/agent-completion-process-monitor.ts:203-207 — the third/fourth `shouldRunCadenceInspection` clauses are only reachable when the option returns `false`.
- src/renderer/src/components/terminal-pane/pty-connection/terminal-keydown-fit.ts:230-242 — the only production coordinator site does not set the option; the comment records the deferred consumption.
- src/renderer/src/components/terminal-pane/agent-completion-no-evidence-cadence.test.ts — three tests exercise a configuration no production caller constructs.
## Open questions for the human
- Is the renderer evidence-consumption (disabling no-evidence polling when the relay publishes `foregroundProcessEvidence`) a separate PR? If so, does the option + tests belong there rather than here?DeepSeek Pro (free via Pullfrog for OSS) ｜ 𝕏
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 86df83bb-928a-4e66-bd1c-a425924358fc
📒 Files selected for processing (6)
src/main/providers/agent-foreground-process-batch.tssrc/relay/pty-handler.tssrc/renderer/src/components/terminal-pane/agent-completion-no-evidence-cadence.test.tssrc/renderer/src/components/terminal-pane/agent-completion-process-monitor.tssrc/renderer/src/components/terminal-pane/pty-connection/terminal-keydown-fit.tssrc/shared/process-table-snapshot.ts
Included review availability: Your plan provides up to 10 included reviews per hour; 8 remain after this review.
| const title = | ||
| (await getForegroundProcessName(managed.pty.pid, managed.pty.process || null)) || 'shell' | ||
| (evidenceRows | ||
| ? (evidenceResults[entryIndex]?.processName ?? managed.pty.process ?? null) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
printf '%s\n' '--- applicable repository guidance ---'
find /tmp/coderabbit-repo-knowledge/stablyai-orca-89dc44e4 -maxdepth 2 -type f -name '*.md' -print
printf '%s\n' '--- changed hunk ---'
git diff -- src/relay/pty-handler.ts | sed -n '/^@@/,$p' | head -240
printf '%s\n' '--- target context ---'
sed -n '2240,2320p' src/relay/pty-handler.tsRepository: stablyai/orca
Length of output: 6747
🏁 Script executed:
for f in /tmp/coderabbit-repo-knowledge/stablyai-orca-89dc44e4/*/*.md; do
case "$f" in
*/learnings/*|*/architecture/*) continue ;;
esac
printf '\n--- %s ---\n' "$f"
head -160 "$f"
doneRepository: stablyai/orca
Length of output: 5356
🏁 Script executed:
printf '%s\n' '--- bound imports and definitions ---'
rg -n -C 5 'getForegroundProcessName|resolveAgentForegroundProcessesBatch|getStrictProcessTableSnapshot|evidenceRows|evidenceResults' src/relay/pty-handler.ts src/relay
printf '%s\n' '--- relay learnings ---'
cat /tmp/coderabbit-repo-knowledge/stablyai-orca-89dc44e4/learnings/src-relay.mdRepository: stablyai/orca
Length of output: 25564
🏁 Script executed:
printf '%s\n' '--- batch resolver implementation ---'
rg -n -C 12 'export async function resolveAgentForegroundProcessesBatch|function resolveAgentForegroundProcessesBatch' src/main/providers/agent-foreground-process.ts
printf '%s\n' '--- strict snapshot contract ---'
rg -n -C 10 'export async function getStrictProcessTableSnapshot|function getStrictProcessTableSnapshot' src/shared/process-table-snapshot.ts
printf '%s\n' '--- legacy foreground resolver ---'
sed -n '215,315p' src/relay/pty-shell-utils.tsRepository: stablyai/orca
Length of output: 4677
🏁 Script executed:
rg -n -C 16 'resolveAgentForegroundProcessesBatch' src/main src/shared src/relayRepository: stablyai/orca
Length of output: 16287
🏁 Script executed:
sed -n '1,125p' src/main/providers/agent-foreground-process-batch.ts
rg -n -C 12 'export function buildProcessTableIndex|function buildProcessTableIndex|export function resolveAgentForegroundProcessesFromIndex' src/shared/process-table-snapshot.ts src/main/providers/agent-foreground-process-batch.tsRepository: stablyai/orca
Length of output: 9079
🏁 Script executed:
sed -n '125,235p' src/main/providers/agent-foreground-process-batch.ts
sed -n '140,190p' src/shared/process-table-snapshot.tsRepository: stablyai/orca
Length of output: 2962
Publish process evidence only after both operations succeed.
If resolveAgentForegroundProcessesBatch rejects after getStrictProcessTableSnapshot() succeeds, the catch leaves evidenceRows populated and evidenceResults empty. The evidenceRows branch then skips getForegroundProcessName and uses only managed.pty.process, which loses the legacy process resolution. Keep both values local and publish them only after both operations succeed.
|
SSH/Electron validation update (HEAD
Readiness remains clean: focused 48-test suite, |
main is red on `static analysis`: oxlint's code-quality pass runs with --deny-warnings, and agent-foreground-process-batch.test.ts imports '../../shared/process-table-snapshot' twice (lines 5 and 13), tripping "Modules should not be imported multiple times in the same file". Introduced by #17525. It blocks every open PR, none of which can go green until this lands.
…ntory path (#17763) Follow-up defect fixes for the batched PTY-inventory evidence path (#17525), now on main. - One memoized `ps` capture serves both the lenient and strict views. The two readers ran byte-identical argv behind separate caches, so a relay serving both forked `ps` twice per 500ms window — the doubling issue #6288 removed. - Drop the `byPgid`/`byTpgid` indexes no resolver reads, plus the zero-caller `parseProcessTableRowsStrict` and `getFreshStrictProcessTableSnapshot`; the batch resolver now reuses the shared index lookup and candidate score instead of private copies. - Restore `getForegroundProcessName`'s ladder contract: the extracted table scan answers null again, so an unconfirmed wrapper fallback publishes the recognized (normalized) name rather than node-pty's raw one. - Pin the SHIPPED `pty.listProcesses` path: one capture and one linear row pass for N panes, and node-pty's own name (never "shell") when the capture cannot disambiguate a `node`/`python` wrapper. - Pin the hidden-pane cadence gate in the production option shape, and move the strict-parser coverage next to the parser it tests.
* feat(ssh): batch process evidence in PTY inventory * fix(ssh): accept Linux kernel process rows and make no-evidence polling push-driven * fix(ssh): preserve process evidence polling semantics --------- Co-authored-by: Merge Sim <sim@local>
…7724) main is red on `static analysis`: oxlint's code-quality pass runs with --deny-warnings, and agent-foreground-process-batch.test.ts imports '../../shared/process-table-snapshot' twice (lines 5 and 13), tripping "Modules should not be imported multiple times in the same file". Introduced by stablyai#17525. It blocks every open PR, none of which can go green until this lands.
…ntory path (stablyai#17763) Follow-up defect fixes for the batched PTY-inventory evidence path (stablyai#17525), now on main. - One memoized `ps` capture serves both the lenient and strict views. The two readers ran byte-identical argv behind separate caches, so a relay serving both forked `ps` twice per 500ms window — the doubling issue stablyai#6288 removed. - Drop the `byPgid`/`byTpgid` indexes no resolver reads, plus the zero-caller `parseProcessTableRowsStrict` and `getFreshStrictProcessTableSnapshot`; the batch resolver now reuses the shared index lookup and candidate score instead of private copies. - Restore `getForegroundProcessName`'s ladder contract: the extracted table scan answers null again, so an unconfirmed wrapper fallback publishes the recognized (normalized) name rather than node-pty's raw one. - Pin the SHIPPED `pty.listProcesses` path: one capture and one linear row pass for N panes, and node-pty's own name (never "shell") when the capture cannot disambiguate a `node`/`python` wrapper. - Pin the hidden-pane cadence gate in the production option shape, and move the strict-parser coverage next to the parser it tests.
Summary
pgid/tpgidand add a strict parser that treats malformed or incomplete captures as unreadable.pty.listProcessesinventory frame while preserving legacy fields and renderer gates.Tests
tsc --noEmit -p config/tsconfig.node.jsonpasses.git diff --checkpasses.