Repository navigation
Conversation
Share target-R preparation between terminal startup and manual attach so console wrappers can use a different R from the background executable. Normalize library path assertions and correct R lint formatting.
eitsupi
changed the title
|
I was wondering whether all vscode-R dependent R packages could use such managed library, separated from user library. Even though it means duplicated copies. |
I agree. jgd is particularly effective because it has no dependencies. |
There was a problem hiding this comment.
🔵 Needs a closer look
The cross-process consent, locking, installation, and namespace-loading lifecycle warrants final human validation despite strong test coverage.
0 open findings
What changed in this PR
Moves ordinary-terminal and manual-attach sess setup into isolated, revision-keyed managed libraries with explicit consent.
Changes:
- Adds shared R-side installation, locking, and namespace-loading logic.
- Adds a TypeScript consent service with persisted prompt suppression.
- Expands lifecycle, concurrency, and integration coverage.
| File | Description |
|---|---|
src/util.ts |
Removes legacy user-library installer flow. |
src/rTerminal.ts |
Passes managed setup resources into R terminals. |
src/session.ts |
Integrates consent with manual attach. |
src/sessConsent.ts |
Implements the consent bridge. |
src/interactive/backends/sessPreparation.ts |
Documents lock compatibility. |
src/test/common/mockvscode.ts |
Updates mocked global state. |
src/test/node/sessConsent.test.ts |
Tests consent behavior and shutdown races. |
src/test/suite/terminal.test.ts |
Tests terminal consent integration. |
src/test/suite/session.test.ts |
Tests attach-script generation. |
src/test/suite/sessionTerminalLifecycle.test.ts |
Updates lifecycle setup. |
src/test/suite/sessInstall.test.ts |
Removes obsolete installer tests. |
src/test/integration/sessInstall.test.ts |
Removes obsolete task-based tests. |
R/attach_sess.R |
Adds shared managed preparation and locking. |
R/install_sess.R |
Requires an explicit target library. |
R/profile.R |
Prepares and loads the selected namespace. |
R/sess_source.R |
Adds runtime identity and explicit loading helpers. |
R/tests/attach_sess.R |
Adds consent and concurrency integration tests. |
R/tests/sess_source.R |
Expands managed-library regression coverage. |
sess/README.md |
Documents isolated installation behavior. |
CONTRIBUTING.md |
Updates contributor guidance. |
package.json |
Runs the new R test suite. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| installed <- sess_find_source_library(expected, managed_library) | ||
| if (length(ready_revision) == 1L && identical(ready_revision, expected) && | ||
| !is.null(installed)) { | ||
| ns <- sess_load_namespace(installed, expected) |
There was a problem hiding this comment.
One potential issue of reusing sess is with renv. If multiple renv projects exist, their isolated environments may not have the dependencies required by sess installed, even if those dependencies are available in another project environment.
There was a problem hiding this comment.
Potentially could be resolved if using one managed library for all vscode-R dependent packages. Leave it to later time then.
renkun-ken
left a comment
renkun-ken
left a comment
There was a problem hiding this comment.
Reviewed 0bc28da. I found one regression in the initial terminal code submission when sess setup takes longer than the existing readiness timeout. The documented isolated-project dependency limitation is already covered by the existing discussion.
Validation passed locally: compilation and TypeScript checking, TypeScript lint, 12 consent-service tests, 6 source/bootstrap tests, both R identity/attach integration suites, 30 core runtime tests, and two real-R library isolation checks. A focused reproduction using the current runTextInTerm implementation and waitForTerminalReady function confirmed the finding below. GitHub build, lint, and all three OS test jobs also pass.
renkun-ken
left a comment
renkun-ken
left a comment
There was a problem hiding this comment.
Reviewed the four new commits through 8b0310d. The previous 30-second setup timeout finding is addressed: I verified that initial input remains queued beyond 35 seconds and is released after setup reports ready and the terminal attaches. The new failure/close/deadline coverage also passes.
One attachment-ordering issue remains: an IPC owner already present at the first readiness call bypasses the startup status check, so a pending startup can release code even if setup subsequently fails. I reproduced this using the real bundled sess and R/profile.R with a controlled runtime_start failure after the attach notification.
Validation passed locally: compilation and TypeScript checking, TypeScript lint, all 71 terminal/lifecycle tests in VS Code 1.140.0, 12 consent tests, six source/bootstrap tests, both R identity/attach suites, and the changed real-R/arf interruption test. GitHub build, lint, and all three OS test jobs pass.
Close #1841
When sess is missing or its source revision differs from the bundled copy, ordinary terminals and manual attach ask before installing into a vscode-R-managed library. Existing user, project, and package-manager installations remain untouched. This provides the package-manager-independent ownership and consent behavior discussed in #1854.
Both paths prepare sess in the R process that will use it. The terminal's startup profile and manual attach share the same R-side helper and extension consent service. This respects the console's actual platform, version, repositories, and libraries even when
r.consolePathselects arf/radian using a different R fromr.executablePath.An exact source-revision match in any normal R library is reused without prompting. Otherwise, an already prepared managed copy is used, or the user can approve installation into a library keyed by R platform, major/minor version, and bundled source revision. The session watcher explicitly loads the selected namespace without changing
.libPaths(), using dependencies from normal libraries where available. Declining installation or a setup failure allows ordinary R to finish starting without the watcher. If extension-side watcher preparation fails, the terminal starts with its ordinary options and a warning, without partial integration settings.Initial code stays queued while the terminal's startup profile reports sess setup in progress (up to ten minutes). Code is sent only after setup succeeds and the attached session matches that terminal's R process and endpoint. Manual attach and automatic reconnection use the same startup helper to publish new setup attempts for the current endpoint, allowing recovery without relying on previous code-send attempts. Declined or failed setup and terminal closure cancel the wait; a terminal with no setup notification retains the existing 30-second timeout. Startup status uses a per-terminal sidecar without changing discovery metadata.
The install prompt offers
Install bundled sess,Not now, andDon't ask again.Not nowand dismissing the prompt decline only that request;Don't ask againsuppresses prompts for the current bundled source revision across sessions. A changed bundled revision prompts again, and suppression always declines installation.Concurrent terminal and manual-attach setup is serialized with a directory lock for each managed library. Managed copies are reused only when the .ready marker and installed package both match the bundled source revision; the marker is published atomically after installation, verification, and namespace loading succeed. Waiting sessions reuse the completed copy, or continue without another prompt if setup was declined. The lock is held through consent, installation, and namespace loading; waits are limited to five minutes, and stale locks are not removed automatically.
A different sess namespace already loaded in R requires a restart rather than forced unloading. Pending grants are denied during extension shutdown or when the session watcher is disabled. The exact
Config/vscode-R/source-revisioncheck and R Interactive's existing isolated runtime lifecycle are preserved. Bundled sess is bumped to3.0.9000.9003;sess::connect()now returns an invisible success flag, while startup errors still propagate. This change adds no settings, package-manager-specific branches, or runtime pruning.Known limitation: a reused managed sess may fail to load when an isolated project lacks its dependencies. The sess README documents the workaround; automatic dependency repair remains outside this PR.