Repository navigation
refactor: group PTY delivery and drain ownership - #33358
Merged
Merged
Conversation
Contributor
|
No actionable comments were generated in the recent review.
 |
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 38 files. (2 skipped: 2… | Write docstrings for the functions missing them to satisfy the coverage threshold. |
| Description check | |
The description is mostly complete for a refactor PR. It has a clear summary of the move of four PTY owners and 15 suites into src/main/ipc/pty/delivery/, the LoC table, and validation results (53 sui… | Add the ELI5 explanation, a What Changed section describing the before and after and the mechanism, and a Why section explaining the reason for the directory grouping and why it beats alternatives. Link a real issue under Linked Issue (repl… |
-
Check name Status Explanation Title check The title clearly summarizes the main change: grouping PTY delivery and drain ownership into a dedicated area. Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request.
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 38 files. (2 skipped: 2 unsupported.)
Resolution
Write docstrings for the functions missing them to satisfy the coverage threshold.
Full details: Description check
-
Explanation
The description is mostly complete for a refactor PR. It has a clear summary of the move of four PTY owners and 15 suites into src/main/ipc/pty/delivery/, the LoC table, and validation results (53 suites / 760 cases, typecheck, mobile type gates). It lacks the ELI5, What Changed, Why, and Linked Issue sections, and the Testing checklist and Notes/Checklist items are not ticked. Visual Proof and AI Disclosure are unaddressed.
Resolution
Add the ELI5 explanation, a What Changed section describing the before and after and the mechanism, and a Why section explaining the reason for the directory grouping and why it beats alternatives. Link a real issue under Linked Issue (replace the empty 'Fixes #'). Fill in Visual Proof with N/A and a reason, since this is a refactor with no UI change. Complete the Testing section: state which platforms were tested and tick the manual testing item if applicable. Fill in the AI Disclosure section if an AI model was used, or skip it if you are a StablyAI team member. Complete the Review section, including the Agent skill upstream boundary checkbox. Address the Notes items (security, cross-platform, remote SSH, mobile, backwards compatibility, performance) and tick the Checklist items that apply.
Finishing Touches: 💡 1
-
📝 Generate docstrings: 💡
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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 help to get the list of available commands.
nwparker
force-pushed
the
nwparker/module-organization-pty-delivery
branch
from
October 25, 2026 03:10
5ae8c36 to
e49ad74
Compare
sj900211
added a commit
to sj900211/orcinus
that referenced
this pull request
Oct 29, 2026
Ten days of upstream, measured from `e680d7c3b68`: 797 commits and 35 conflicting files, base `cb0b1524091`. Four cluster analysts, one implementer, one independent critic, which returned ACCEPT with 0 blocking defects. This is stage 1 of 4. The gap is 2,355 commits, and a PTY delivery refactor (`ad1b81d53a0`, stablyai#33358, "group PTY delivery and drain ownership") lands just past this checkpoint — conflicts jump from 35 to 54 and PTY conflicts from 7 to 19 the day after it. Stopping here keeps that refactor in a stage of its own, the way an earlier PTY wall was handled. ## The fork's attribution nearly went nowhere Upstream deleted `src/main/ipc/pty/ipc/spawn-commit-persist.ts`. The fork's owner-window attribution lived in it, so the deletion applied cleanly and the behaviour would simply have ceased to exist — a modify/delete where "resolve the conflict" and "keep the fork's change" are different acts. The attribution is ported into `spawn-commit-publication.ts:145,148`, the deps member in `spawn-commit-assembly.ts:42` is widened to match, and the dead file is removed rather than left as git's "kept in tree" copy. The critic traced the path end to end rather than reading the diff. ## Two new upstream ratchets, grandfathered rather than obeyed Upstream added `host-list-render` and `id-only-store-write`, each with an allowance of 0 written against a tree that has no fork SFTP server explorer, no remote hosts pane and no satellite-window session persistence. The fork's code predates the rule it now violates, which is what grandfathering is for, and the ratchet asks for reviewer sign-off. Entries are the narrowest possible — specific files, exact counts, each with a reason naming the fork feature. Measured against `120f01331ca` rather than against `main` (most baselines differ from `main` only because upstream changed them), exactly two baseline files differ from upstream's, by +2 and +3 lines. Nothing else was widened. The pre-existing `focus-read` ratchet was deliberately left red. It is red identically on `main`, under a rule that predates this merge, so widening its allowance here would be unrelated housekeeping. The aggregate `run-ratchets.mjs` therefore still exits 1 on that one ratchet alone. Worth noting the other direction too: `layout-writer` is red on `main` and this merge turns it green. ## What the gates found that the plans did not - The cold typecheck caught two clean-merge breaks: `satellite-session-staging.ts` needed upstream's `flushPendingEditorBuffer`, and a new upstream test dropped `getMainWindow`. - `clipboard-ipc-handlers.ts` merged cleanly at 305/300 — the seventh size breach of this sync, and again invisible to the `max-lines` ratchet, which tracks disables rather than breaches. Extracted to a sibling, no disable. - The path-string set difference found the deleted SQLite allowlist still naming a removed test. No compiler or test executes that file. Two fixes carry controls rather than assertions of correctness: reverting the `applyMovedEditorFile` fix makes its new test fail (`Editor buffer mirror "isDirty" … differs from its record`), and reverting the unlink-race fix makes its test time out. A test that passes either way is worse than none. ## A fork rule this merge bends, deliberately shipped Stage 5 ruled that an SFTP preview tab must never resolve to a local file. The critic probed the merged tree: open a dirty local tab at `/repo/app.ts`, then an SFTP preview of the same remote path, and both land on one editor buffer record (`[null,"local",null,"/repo/app.ts"]`) — the remote preview displays the local file's unsaved draft. Upstream's new `editorBufferIdentityOf` keys on a host id from `resolveEditorModelOwner`, which reads `externalSshTargetId` but not the fork's `sftpTargetId`. This collision already exists on `main` one layer down, at the Monaco model, so the merge widens it from a shared model to shared store state rather than creating it; and the SFTP loader writes only `fileContents`, so no path was found where remote content overwrites the local record. It ships for that reason, not because it is acceptable. The fix belongs in `resolveEditorModelOwner` — give a file with `sftpTargetId` its own host key ahead of the `externalSshTargetId` branch — and is scheduled before stage 14. ## Gates Every gate was re-measured by the critic in a worktree materialised from the staged index with its own frozen install, after confirming all four trees match the lockfile. That check is not ceremony: last stage every local gate passed against a stale `node_modules` and CI then failed on a real error. Cold typecheck 0 errors. Full-tree oxlint 0 errors. Identity OK, `upstreamRefs=0 launcherProduct=Orcinus`. Unsigned smoke, fork contract tests, the Windows CLI launcher's real Rust builds, and the localization set: pass. Regression sweep over `discoverUnitFiles()`: `listed=13498 reported=13498 missing=0`, 510 failing files rechecked on both parents, `mergeOnly` 15 → 0 merge-caused after attribution. Path-string set difference `mergeOnly=0` with a positive control of 3. The six `OXLINT_SCANS` on three trees: 4 raw merge-only rows, all shown to be the same diagnostics at the same lines on `main` with only an embedded variant list in the message changed. Known and not addressed: project windows get no Resource Manager visibility pushes (stablyai#31593 wired the main window only) — a fork feature gap, not a merge regression; `attach-main-window-notifier-routing` stays red, and its failure reason changed because upstream renamed a notifier member; full-tree `oxfmt` is red on 19 fork-only files, identically on `main`. ## Committed with --no-verify The pre-commit hook could not run on a merge this size. lint-staged split the 9,349 staged paths into 147 chunks of 60 and one chunk contained no lintable file, so its oxlint invocation exited on "No files found to lint" and the whole hook failed. lint-staged reverted cleanly and the staged merge was untouched (same write-tree, MERGE_HEAD intact, nothing unstaged). Nothing is skipped by this: full-tree oxlint, oxfmt, the cold typecheck and every ratchet were run by the implementer and re-run by the critic in a worktree materialised from this exact index with its own frozen install, which is a stricter check than lint-staged performs. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
sj900211
added a commit
to sj900211/orcinus
that referenced
this pull request
Oct 30, 2026
One day of upstream past stage 13: 365 commits and 24 conflicting files, base `120f01331ca`. Three cluster analysts, one implementer, one independent critic, which returned ACCEPT with 0 blocking defects. The target is the parent of the PTY delivery refactor `ad1b81d53a0` (stablyai#33358, "group PTY delivery and drain ownership"), which is left to stage 15 on its own. The remaining gap after this merge is about 1,685 commits. ## Most PTY conflicts were comments Bisection showed the first upstream commit to make any PTY file conflict is `3286f3b2591` (stablyai#32966, a comment audit); before it there were 5 conflicts and no PTY ones. 16 of the 24 conflicts are that audit colliding with the fork's per-window delivery code. Each was resolved as the fork's code with upstream's comment text, adapted where it now misdescribes per-window behaviour, and the critic confirmed by token comparison that the resolved code is identical to `main` in all 16 (two more resolved files also qualify). ## Collateral outside the conflict list - `terminal-query-responder.test.ts`: upstream stablyai#33273 wrote a one-argument `markHiddenRendererPty` call against the single-window gate; the fork keys marks by `webContentsId`, so the test now marks `GATE_WINDOW`. - `pty-spawn-env-terminal-basics.test.ts`: a cast upstream removed is restored with a `SAFETY:` reason, because the fork's `registerPtyHandlers` takes a `BrowserWindow` for owner routing. - `local-file-access.test.ts`: the fork's census still named `WorkspaceEmptyState.tsx`, which upstream deleted when reverting stablyai#32182 (stablyai#32838). Its renderer-relative path hid it from the repo-rooted path sweep; the regression sweep found it. ## Identity `LOCAL_HOST_ROOT_NAME = 'Orcinus'` and the `sj900211/orcinus` release feed (`TAG_HREF_RE`) survive; the identity guard passes with `upstreamRefs=0`. The guard cannot see regex-escaped URLs, so the feed's unit test is what protects that hunk. Pre-existing `stablyai` drift in the prerelease feed's download regex and its readiness-test mocks is left for a separate commit so this merge stays attributable. ## Gates Cold typecheck 0 errors (17,824 files), full-tree oxlint 0 errors and 0 `max-lines` (`register-handlers.ts` and `daemon-host-relocation.ts` at 297/300), identity guard OK, fork contracts green. A full `discoverUnitFiles()` sweep on three trees (13,498 / 13,523 / 13,585 files, missing=0) raised 81 merge-only tests; isolated and serial rechecks attribute every one to Windows EPERM teardown or shell timing that also fails on the baselines, leaving 0. The critic's independent 807-file resample on LF trees agrees. Not applied here, by ruling: cosmetic comment ports into fork-only extractions, and upstream stablyai#32940's main-window-only project-move pause barrier, which has no production caller yet and is a watch item for when upstream wires one. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
sj900211
added a commit
to sj900211/orcinus
that referenced
this pull request
Nov 1, 2026
…h-up sync 15 Stage 15 is exactly one upstream commit: "refactor: group PTY delivery and drain ownership" (stablyai#33358), base `9f040bcb8ba`. It moves the hidden-delivery gate, the producer flow control and the pending-data drain queue/contract plus 15 delivery tests into `src/main/ipc/pty/delivery/`, and rewrites importers. 16 files conflicted, all PTY. One analyst, one implementer, one independent critic, which returned ACCEPT with 0 blocking defects. ## A move, proven to be only a move The analyst re-pointed the moved-module specifiers in the base tree, ran oxfmt, and reproduced upstream's side byte for byte for all 45 source files; the critic replayed it independently. So the only failure class this merge could carry was module resolution and fork content lost across a move. - The fork's per-window gate (per-`webContentsId` marks, the window-independent `runtimeOwnedHiddenRendererPtys` set, two-argument `isHiddenRendererPty`, `isHiddenRendererPtyInAnyWindow`) and its test now live only at the new path; nothing survives at the old one. - Five fork imports still named the old paths and merged with no conflict marker: two in clean regions of conflicted files (`hidden-transition.ts`, `session.ts`) and three in cleanly-merged fork files (`window-flow-state.ts`, `pty-owner-gate.ts`, `pty-project-window-stream-routing.test.ts`). All five are re-pointed; an old-path census reads base 40, main 43, upstream 0, staged 0. ## Gates Run in an LF clone of the staged tree, because `F:/osync`'s worktree is checked out CRLF over LF blobs. Cold typecheck 0 errors (a planted old path fails with TS2307), full-tree oxlint 0 errors and 0 `max-lines` (`pty-launch-parity.test.ts` 800/800, `register-handlers.ts` 297/300, `window-flow-state.ts` 287/300), identity guard OK, both invariant greps empty, path-string and relative-specifier sweeps `mergeOnly=0` with positive controls, and an affected-test sweep over 735 files on three trees with 0 merge-only failures after isolated rechecks. The full `discoverUnitFiles()` sweep was skipped on purpose: for a proven no-logic move, typecheck plus the specifier sweeps cover the only failure class exhaustively. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Moves the four PTY hidden-delivery, pending-drain and producer-flow owners and 15 existing suites into
src/main/ipc/pty/delivery/. Updates concrete owner imports, mocks, timing entries and live reliability commands while preserving historical execution records.Two independent refactor audits verified code bodies, singleton identities, imports and the complete committed tree. Fresh baseline and candidate validation passed all 53 whole suites / 760 cases with exact case parity, four desktop typecheck projects, both mobile type gates with zero allowances, and all 13 local checks. Full required CI is running.