feat(workspace): offer to install the engine a bound workspace needs - #1158
feat(workspace): offer to install the engine a bound workspace needs#1158ralphstodomingo wants to merge 11 commits into
Conversation
|
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: trueThanks 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 |
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 45e30dece9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| function printLine(line: string): void { | ||
| if (syncInternals.printLine) return syncInternals.printLine(line) | ||
| try { | ||
| process.stdout.write(line + "\n") |
There was a problem hiding this comment.
When run --format json is used in a bound workspace with a missing or outdated engine, this writes a human-readable line directly to stdout, while run.ts promises raw JSON events and serializes its other stdout records through emit(). The resulting stream contains a non-JSON line, breaking line-oriented JSON consumers and CI automation; emit a structured JSON record in JSON mode or send this notice to stderr.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in 0d942e6.
Reproduced before changing anything: run --format json in a bound workspace with no engine on PATH produced 5 stdout lines, of which line 1 was the plain notice and the other 4 were valid JSON. So the framing break is real, not theoretical.
Fixed by sending the notice to stderr rather than emitting a JSON record. It is a status notice rather than run output, which is the same reason run already writes its own > builder · <model> status line to stderr — so stderr is correct in both formats, not just a JSON-mode workaround. JSON stays parseable, and a terminal user still sees the notice in either format.
Verified after: JSON mode is 4/4 valid JSON on stdout with the notice on stderr once; text mode still prints it exactly once. Added a regression test that drives the real printer (not the seam) and asserts the line reaches stderr and never stdout.
808282a to
83d0636
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0d942e6b34
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const spec = installSpec() | ||
| if (syncInternals.install) return syncInternals.install(spec) | ||
| return new Promise((resolve) => { | ||
| execFile("npm", ["i", "-g", spec], { timeout: 300_000 }, (err, _stdout, stderr) => { |
There was a problem hiding this comment.
On Windows, a normal Node installation exposes npm as a command shim rather than an npm executable, so execFile("npm", ...) fails with ENOENT when the user selects Install now, despite the Node-version gate succeeding. The existing install path in packages/opencode/src/lsp/server.ts:213-215 already handles npm as platform-specific; use a Windows-compatible invocation such as cmd.exe /c npm.cmd here as well.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in 1babc37.
The repo already agrees with you: lsp/server.ts:213 does process.platform === "win32" ? "npm.cmd" : "npm" for exactly this reason. execFile spawns no shell, so the bare name would ENOENT on the one platform where the Node gate had just told the user they were good to go.
Fixed via the house Process.run helper with the same platform split rather than cmd.exe /c. Two reasons: it matches the existing precedent in this codebase, and Process.run takes an argv array, so an install spec containing spaces — the local tarball path E2E uses — needs no quoting. (npm.cmd through cmd.exe /c would have needed care there.)
Re-ran the install E2E on Linux after the change since the mechanism moved: dialog to "Install now" to engine 0.7.0 installed into an isolated npm prefix, success toast, tools on the next message. Windows itself is unverified — I have no Windows host — so this rests on matching the existing precedent rather than on a test.
| // altimate_change — carries the fail-open notice when the target could not be | ||
| // attributed to the workspace; a no-op otherwise. | ||
| return Precedence.annotate(precedence, { | ||
| title: `SQL: ${args.query.slice(0, 60)}${args.query.length > 60 ? "..." : ""}`, | ||
| metadata: { rowCount: result.row_count, truncated: result.truncated }, | ||
| output, | ||
| } | ||
| }) |
There was a problem hiding this comment.
When precedence returns an undetermined fail-open verdict—such as a default dbt adapter whose type cannot be identified—this annotation is applied only to the successful result. If local execution then throws, the catch path returns an unannotated error, losing both the user-facing reason that routing was skipped and the precedence telemetry marker. The same omission exists in the failure paths of sql-explain.ts and schema-inspect.ts; annotate failure results too so the promised non-silent fail-open behavior survives execution errors.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed as a real gap, but this one is not mine to fix and I am routing it rather than touching it.
sql-execute.ts, sql-explain.ts and schema-inspect.ts are precedence code from the PR below this one in the stack (#1156), not the install offer. This PR changes engine-sync.ts, the workspace TUI plugin, and one env marker in run.ts; it adds no annotation and no precedence path. Fixing it here would put a precedence change in an install-offer PR and split ownership of that code across two PRs.
I have passed it to the session that owns #1156 with your reasoning intact: the undetermined fail-open verdict is annotated only on the success path, so a throw from local execution returns an unannotated error and loses both the user-facing reason routing was skipped and the precedence telemetry marker — and the same omission is in the failure paths of the other two tools.
Flagging for whoever reads this thread: if #1156 lands the fix, it arrives here through the stack rather than as a commit on this branch.
️✅ There are no secrets present in this pull request anymore.If these secrets were true positive and are still valid, we highly recommend you to revoke them. 🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request. |
|
Thanks for updating your PR! It now meets our contributing guidelines. 👍 |
83d0636 to
e772cb7
Compare
6ab3731 to
3caa27f
Compare
Codex review logKept out of the PR description so the body stays inside the repo's 5,000-char limit. Edited in place as rounds land. Round 1 — 1 finding
Reproduced first: Round 2 — 2 findings
The npm fix follows the split already in this repo rather than the suggested The second finding is precedence code from the PR below this one in the stack. Codex reviews the whole branch diff, so those files are in its view; fixing them here would have split ownership of that code across two PRs. It was routed, confirmed real, and found to be wider than reported — six unannotated exits rather than three — and is fixed on that branch. Round 3 — 2 findings, both mine
The first was mine to introduce: the original used a call whose timeout kills the child, and moving to the shared helper for the Windows fix silently lost that, because the helper consults its timeout only inside its abort handler as the grace before SIGKILL. Measured rather than reasoned about, same helper and an 8s sleep — timeout alone ran 8004ms, an abort signal killed at 502ms. A stalled install now reports that it did not finish, and the install E2E was re-run afterwards to confirm a normal install is unaffected. The second has an exact precedent: the bash tool already strips the non-interactive marker from child environments for the same reason, and the headless marker was not being stripped alongside it. Review auditRe-audited rather than assumed closed, since a verdict can arrive as inline comments, a review body, or a reaction, and inline comments can land after the body. On this PR: 5 inline findings across 3 rounds, every review body boilerplate with no findings inside, no reaction-only verdict, and 5 replies — one per finding. 4 fixed here, 1 routed to the precedence PR and fixed there. A known divergence, stated deliberately
Round 4 — 1 finding
Escape or a click outside dismisses the offer while npm keeps running. The failure path set signals on an unmounted component, so an npm error — or the five-minute timeout added in round 3 — reached nobody. That is the worse half: the timeout exists to say the install gave up, and it was silent in exactly the case where the user had stopped watching. Success also cleared the dialog stack unconditionally, which would close a dialog the user had opened since. Fixed by tracking mount state: completion reports through a toast when the dialog is gone, carrying the error and the command to run by hand, and the dialog is cleared only while this offer still owns it. Verified in a real TUI — Install now, Escape while npm ran, no dialog rows left, install completed and the result still surfaced. That path produced nothing at all before. On reading the verdict. The reaction on the summon comment was 👀, not 👍 — "looking", not "nothing found" — and the finding arrived about three minutes after it. Reading the reaction as a verdict would have produced a "round 4: no findings" report with a real P2 sitting unaddressed. Surfaces were compared by identifier before and after the summon rather than by count, and watched past the first signal. Round 5 — 1 finding
Two dispatches arriving inside that window both pass, and the failure mode is worse than the bug the guard was written for — a second dialog replacing an installing one can start a concurrent global npm install. The slot is now reserved before the first await and released on the suppressed and failed paths, and each raise carries an ownership token so a superseded dialog tearing down cannot free a slot the newer one holds. No unit test for the interleaving itself: driving two concurrent dispatches through the plugin surface would have been less convincing than the structural change, so that is stated rather than implied. A bug found by testing rather than by reviewBetween rounds 4 and 5, the install → next-message row stopped completing on this base. It looked like an input-delivery flake, and had it been reported that way it would have been wrong. Capturing the pane instead showed a second offer dialog, in its idle phase, sitting over the session after the install finished — attach re-probes a repairable failure every turn, so the offer was being raised again mid-install, replacing the "Installing…" dialog and swallowing keystrokes into its own filter. That produced the single-offer latch, which round 5 then correctly identified as racy. Worth recording because the earlier bases passed this row: the retry behaviour that makes the re-raise likely arrived underneath this PR, so evidence gathered before it proved nothing about after it. Rounds 6–8
Round 6 is a distinct latch from round 5's: that one stopped two raises interleaving, this one is a raise that is legitimately alone but arrives while an install started by an already-dismissed dialog is still running. The dialog latch answers "is an offer on screen"; a second latch now answers "is an install running", and only the second survives dismissal. Round 7's P1 is not fully closed and should not be read as such. The harmful outcome is gone — the offer refuses to act when the server's directory is absent locally, which is the same signal Round 8 was the worst of the three surfaces: the local headless path prints, the TUI shows a dialog, and an attached run showed nothing at all. The run event loop now renders it on stderr, and the local path was regression-checked afterwards — still exactly one line, no duplicate. Two fixes in these rounds were wrong on the first attempt and only testing caught it. The filter fix landed on a different dialog in the same file that matched the same three lines of JSX: typecheck passed, 311 tests passed, and the rows were still being filtered when the scenario was re-run. The single-offer latch of round 5 was itself introduced to fix a bug found by testing, then found racy by review — the two modes catch different things and neither would have sufficed alone. Rounds 9–12
Round 9's first finding is one this suite's own E2E could not have caught: every install run here put the isolated prefix's bin directory on PATH, so the installed engine was always discoverable. Reproducing it required deliberately leaving it off, and the dialog then claimed success for an engine nothing could find. Round 10 is fixed only for an offer arriving in the same batch as idle. One published strictly after idle still cannot be rendered in an attached run, because the stream is finished — closing that needs either foreknowledge the client does not have or a grace window on every run's exit. The other two surfaces are unaffected. Test dedupe406 → 358 lines, 22 → 18 tests. Removed two that asserted a stub's own return value — both would have passed with the function they named deleted — and one that only checked a constant is positive. The TUI routing invariant was covered twice, through the seam and through the real publish path; the real one stays. Every survivor was then mutation-checked rather than assumed to bite. Each of these turned at least one test red, and the source was restored byte-identical afterwards:
Second UI capture — no Node on PATHInstall now is absent and the reason is shown: Verification detailUnit 262 pass / 0 fail on the workspace + plugin suites; full E2E against a real bound workspace with the engine absent from PATH, headless runs with stdin closed: dialog with all three options and the live declared count; Install now installs into an isolated prefix and the tools appear on the next message in the same session; Not now suppresses and returns after the latch is aged 8 days; no Node hides Install now; headless prints exactly one line, identical across 3 runs. Verification on the merged baseRe-run of the install row on
That last point is why the post-install message says tools attach on your next message rather than your next session: the attach re-probes a repairable failure each turn, so the engine installed here is picked up without restarting. Separately, the attach behaviour underneath this was verified on a clean build of the attach branch with none of this PR's changes applied — engine absent at turn 1, installed by hand mid-session, tools present at turn 2 — so the repair is the attach layer's, not something this dialog papers over. Re-verified on the final baseThe base moved once more (precedence round 5). Rather than carry the previous result forward, the install row was re-run on this head: dialog with all three options, Install now completes, engine reports 0.7.0 in an isolated prefix, and The number of tool names visible differed between runs (12 earlier, 5 here). That is a screen capture of a reply still streaming, not a measurement — the assertion is that engine tools are present in the same session after installing, which held on both. Nothing in this PR affects how many tools a workspace serves. This PR adds no permission surface: it registers no model-facing tool, redirects no call, and changes no ruleset. Its only outputs are a dialog, a toast, a stderr line, and an npm invocation made on an explicit choice — so the redirect-across-a-permission-boundary trap that shaped the precedence work does not apply here. |
3caa27f to
881cb9e
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 881cb9ed3d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (syncInternals.install) return syncInternals.install(spec) | ||
| const npm = process.platform === "win32" ? "npm.cmd" : "npm" | ||
| try { | ||
| const result = await Process.run([npm, "i", "-g", spec], { timeout: 300_000, nothrow: true }) |
There was a problem hiding this comment.
When npm stops making progress, this timeout does not terminate it: Process.spawn only uses that option as the grace period after its abort() handler is invoked, while this call supplies no abort signal. Consequently installEngine() can remain pending indefinitely and leave the dialog stuck on “Installing…” rather than failing after five minutes; provide a timed abort signal or implement an actual execution deadline.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in ce6cb79 — and this was a regression I introduced, not a pre-existing gap.
The original used execFile, whose timeout does kill the child. I moved to Process.run for the Windows shim fix in the previous round and silently lost the deadline, because Process.spawn consults timeout only inside its abort handler, as the grace period before escalating to SIGKILL. With no signal supplied that handler never runs.
Measured it rather than reasoning about it — same helper, same options, an 8 second sleep:
{ timeout: 500 }alone: 8004ms (ran to completion, no deadline){ abort: AbortSignal.timeout(500) }: 502ms
Fixed with a real abort signal, and a stalled install now reports "npm did not finish within 5 minutes" rather than leaving the dialog on "Installing…". Re-ran the install E2E afterwards to confirm the abort signal does not disturb a normal install: dialog to Install now to engine 0.7.0 in an isolated prefix, unchanged. Regression test added with the measured numbers in the comment so the next person does not repeat the swap.
| // a dialog or a toast, so the workspace engine offer degrades to a single | ||
| // printed line. An env var because it must be readable from every module | ||
| // realm (the TUI plugin runtime loads plugins in its own). | ||
| process.env["ALTIMATE_CODE_HEADLESS"] = "1" |
There was a problem hiding this comment.
When a run session launches another altimate-code entrypoint such as a TUI or serve through the bash tool, that child inherits this process-wide marker because bash.ts strips ALTIMATE_NON_INTERACTIVE but not ALTIMATE_CODE_HEADLESS. The nested server then makes isHeadless() true, so offerOrNotify() bypasses TuiEvent.CommandExecute and writes the engine notice to stderr instead of showing the interactive install dialog; remove this marker from tool child environments just like the existing non-interactive marker.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in ce6cb79.
Verified the precedent you point at: bash.ts:178 already does delete mergedEnv["ALTIMATE_NON_INTERACTIVE"], with a comment explaining that a child spawned by the bash tool may itself be a server-mode entrypoint that needs the path the parent disabled. ALTIMATE_CODE_HEADLESS is exactly the same class of marker and was not being stripped, so a nested entrypoint launched from a run session would inherit "headless" and print the engine notice to stderr instead of showing the install dialog.
Fixed alongside the existing delete, with a comment tying the two together so they stay in step. Good catch — the marker only exists because the offer needs to know it has no TUI to render into, and the one case where that inference is wrong is precisely a child process that does have one.
ce6cb79 to
03f1218
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 03f121823b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (!result.ok) { | ||
| setFailure(result.error) | ||
| setPhase("failed") |
There was a problem hiding this comment.
When a user selects Install now and then dismisses the dialog with Escape or an outside click while npm is still running, the install continues but this failure path only updates signals belonging to the now-unmounted component, so an npm error or five-minute timeout is never shown. A successful completion can also call dialog.clear() after another dialog has opened. Keep the operation's completion handling outside the dismissible component, or emit result toasts and clear only when this offer still owns the dialog.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in the head above. Both halves were real.
Failure path: setFailure / setPhase after dismissal update signals on an unmounted component, so an npm error — or the five-minute timeout added in the previous round — reached nobody. That is the worse half, because the timeout exists precisely to tell the user the install gave up, and it was silent in exactly the case where they had stopped watching the dialog.
Success path: dialog.clear() ran unconditionally, so if the user had opened another dialog while npm ran, we would have closed theirs.
Fixed by tracking mount state with onCleanup: completion reports through a toast when the dialog is gone (carrying the error and the command to run by hand), and the dialog is cleared only while this offer still owns it.
Verified in a real TUI rather than by reading: Install now, Escape while npm was still running, dialog gone with no rows left — the install completed and the result still surfaced, and the engine landed in the isolated prefix. Before this change that path produced nothing at all.
7fef85c to
19cb5e5
Compare
6cc2b33 to
ff9b5b6
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ff9b5b6ef2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (engineOfferVisible) return | ||
| const offer = await describeOffer(api.state.path.directory) |
There was a problem hiding this comment.
When two sessions or repair retries dispatch this command close together, both handlers can pass this guard while the first is awaiting describeOffer, credential lookup, or Node detection. The later handler can then call dialog.replace() after the user has started installing from the first dialog, replacing the installing state with a fresh idle offer and allowing a second concurrent global npm install; cleanup of the first dialog can also reset engineOfferVisible while the second remains mounted. Set the in-flight latch before the first await and release it when discovery is suppressed or fails.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in the head above. You are right, and the guard I added a commit earlier was worse than I thought.
Check-then-act: the latch was read, then describeOffer, the credential lookup and Node detection all awaited, and only then was it set. Two dispatches arriving inside that window both pass. And as you note, the failure mode is worse than the one the guard was written for — a second dialog replacing an installing one can start a concurrent global npm install, where the original bug merely swallowed keystrokes.
Fixed by reserving the slot before the first await and releasing it on the suppressed and failed paths. The teardown race you flagged is handled with an ownership token rather than a bare boolean: each raise takes a generation number, and a dialog releases the latch on cleanup only if it still owns it, so a superseded dialog tearing down cannot free a slot the newer one holds.
Verified end to end after the change on an isolated project with the engine absent from PATH: exactly one offer on screen, install completes, no dialog rows left afterwards, the next message reaches the prompt, and engine tools attach in the same session. No unit test for the interleaving itself — I did not find a way to drive two concurrent dispatches through the plugin surface that would be more convincing than the structural change, so I am flagging that rather than implying coverage I do not have.
19cb5e5 to
8740567
Compare
Replaces the transient toast for a missing or too-old engine with Install now / Copy command / Not now. The offer reaches the TUI on the event bus, since plugins load in a separate realm; the plugin re-derives detail via describeOffer(). Headless run prints one line to stderr so --format json stays parseable. Install runs only from an explicit choice.
Process.spawn consults `timeout` only inside its abort handler, as the grace before SIGKILL, so with no signal there was no deadline and a stalled npm left the dialog on "Installing..." indefinitely. Measured: an 8s sleep ran 8004ms under `timeout`, 502ms under an abort signal. The bash tool now also strips ALTIMATE_CODE_HEADLESS from child environments, as it already does for ALTIMATE_NON_INTERACTIVE.
Escape or a click outside dismisses the offer while npm keeps running. The failure path only set signals on the unmounted component, so a failed install or the five-minute timeout was completely silent; success also cleared the dialog stack unconditionally, which would close whatever had opened in its place. Completion now reports through a toast when the dialog is gone, and only clears a dialog this offer still owns.
Attach re-probes a repairable failure every turn, so the offer could be raised again while one was still up. Mid-install that replaced the "Installing..." dialog with a fresh idle one, which swallowed keystrokes into its own filter — observed end to end: after a successful install, typing never reached the prompt. The offer is now latched while one is on screen.
The single-offer guard checked the latch and then awaited three times before setting it, so two dispatches arriving close together could both pass — worse than the bug it fixed, since the second dialog can replace an installing one and start a concurrent global npm install. The slot is now reserved before the first await and released if discovery is suppressed or fails, and only the raise that owns the latch may free it.
40308bc to
8992318
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 89923187e6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // Release the single-offer latch however this dialog goes away — chosen, | ||
| // dismissed, or replaced — but only if this dialog still owns it. A | ||
| // superseded dialog tearing down must not free a slot the newer one holds. | ||
| if (engineOfferGeneration === props.generation) engineOfferVisible = false |
There was a problem hiding this comment.
When the user selects Install now, dismisses the dialog while npm is still running, and then sends another message, this cleanup releases the only global offer latch even though runInstall() continues in the background. The repair retry can consequently open a fresh offer and allow a second npm i -g against the same global prefix. Fresh evidence beyond the previously fixed concurrent-offer race is this dismissal path explicitly setting engineOfferVisible to false before the install promise settles; keep a separate install-in-flight latch until that promise completes.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in the head above. This is genuinely distinct from the round-5 race, and you are right that the previous fix did not cover it.
Round 5 stopped two raises from interleaving. This is a raise that is legitimately alone, but arrives while an install started by an already-dismissed dialog is still running — the dialog latch was released by cleanup, because it answers "is an offer on screen", and after dismissal the honest answer is no.
Fixed with a second, independent latch that answers "is an install running" and is held across the whole installEngine() call in a finally, so it survives the dialog it started in. Raising an offer is suppressed while it is set.
Verified end to end on the exact path you describe: Install now → Escape while npm was still running → dialog gone with zero rows → sent another message in the same session → no second offer was raised, and the install completed normally with the engine landing in the isolated prefix. I also checked for concurrent installs by process; the count I first saw was my own shell matching the pattern, not a second npm.
Dismissing mid-install tears the dialog down and freed the only offer latch while npm kept running, so the next turn's repair retry could raise a fresh offer whose Install now started a second global install against the same prefix. The dialog latch answers "is an offer on screen"; a separate latch now answers "is an install running", and only the second survives dismissal.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a1795ec986
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (engineOfferGeneration === generation) engineOfferVisible = false | ||
| } | ||
| try { | ||
| const offer = await describeOffer(api.state.path.directory) |
There was a problem hiding this comment.
When a TUI uses altimate-code attach with a remote server, this plugin executes in the local TUI process, while the workspace binding, engine PATH, and MCP session belong to the server. Re-deriving the offer here therefore reads the client's filesystem and PATH—often returning null because the remote directory does not exist locally—and, if it does render, installEngine() later installs npm on the client rather than the server. The remote workspace remains without an engine despite a possible success toast, so discovery and installation need a server-side API rather than direct calls from the TUI plugin.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed as real, and partially fixed — I want to be precise about which part, because the complete answer is out of scope for this PR and I would rather flag that than imply it is closed.
You are right about the mechanism. attach <url> runs this plugin in the client TUI process while the binding, the PATH that matters, and the MCP session all belong to the server. attach.ts itself recognises the case the same way, at the point where it passes a directory through when it does not exist locally.
What is fixed: the offer now refuses to act when the directory the server reports is absent on this machine, and says the engine is missing on the server with the command to run there. That removes the harmful outcome you identified — a local npm install behind a success toast while the remote workspace stays exactly as it was.
What is NOT fixed, and is stated in the code and the PR: a client that happens to have the same path present, with a binding, is still misread. Closing that properly needs server-side discovery and install behind an API, which this PR does not add and which I am not going to bolt on at this stage of review — it is a larger change than the feature it would be guarding.
Untested on real remote attach: I have no second host, so this rests on the directory signal that attach.ts already uses rather than on an observed remote session. Flagging that rather than claiming verification I do not have.
| <props.api.ui.DialogSelect | ||
| title={title()} | ||
| options={options()} | ||
| current={canInstall() ? "install" : "copy"} |
There was a problem hiding this comment.
When a user types a filter such as now before selecting Install now, the intentionally persistent DialogSelect retains that filter while options() changes phases. The installing sentinel and, after an npm failure, the Copy command and Close rows do not match that query, so DialogSelect.filtered() becomes empty and the recovery actions cannot be selected. Disable filtering for this fixed three-option dialog or clear its filter whenever the phase changes.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed. skipFilter exists on this component and filtered() short-circuits on it, so the dialog now opts out of filtering entirely — it has three fixed options and gains nothing from a query.
Verified the trap and the fix in a real TUI rather than reasoning about it: with the offer up, typing now used to leave 2 of 3 rows; it now leaves all 3, and selecting still starts the install, which completes with the engine landing in the isolated prefix.
Worth recording that my first attempt at this fix was wrong in a way the type checker and the suite both accepted: the attribute landed on a different dialog in the same file that happened to match the same three lines of JSX. It typechecked, 311 tests stayed green, and only re-running the actual scenario showed the rows still being filtered.
`attach <url>` runs the plugin on the client while the binding, PATH and MCP session live on the server, so Install now would install on the wrong machine behind a success toast. The offer now refuses when the server's directory is absent locally and says where the fix belongs. Also drops the select filter, which persisted across phase changes and stranded the recovery rows.
c88718e to
9e66e77
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9e66e77d44
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // a dialog or a toast, so the workspace engine offer degrades to a single | ||
| // printed line. An env var because it must be readable from every module | ||
| // realm (the TUI plugin runtime loads plugins in its own). | ||
| process.env["ALTIMATE_CODE_HEADLESS"] = "1" |
There was a problem hiding this comment.
When run --attach <url> targets a server whose bound workspace has a missing or outdated engine, this marker exists only in the local CLI process; the attach flow and isHeadless() execute in the already-running server process. The server therefore publishes tui.command.execute and treats that successful publish as delivery, but the run.ts event loop has no handler for that event, so both default and JSON-mode users receive no install notice at all. Pass the client capability to the server or render the returned offer event in the attached run client.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in the head above. Verified the mechanism in source before changing anything rather than taking the description on trust:
rundoes have--attach(the option is declared, and a comment a few lines below notes the agent runs on the remote server when it is set).- The event loop handles
message.updated,message.part.updated,session.error,session.statusandpermission.asked— and nothing else. There was indeed no handler fortui.command.execute.
So with --attach the marker is set on the local process while isHeadless() runs server-side and returns false; the server publishes the offer command, treats the publish as delivery, and the only surface the user is actually watching drops it silently. That is the worst of the three shapes — the local headless path prints, the TUI path shows a dialog, and this one showed nothing at all.
Fixed by rendering it in the run event loop, on stderr for the same reason the local notice goes there: stdout is raw JSON events under --format json.
Regression-checked the local path afterwards, since both now describe the same situation: a normal headless run still emits exactly one offer line on stderr and no duplicate from the new handler.
Not verified against a real attached server — I have no second host — so this rests on the source reading above rather than an observed remote run.
With `run --attach`, the headless marker is set on the local CLI process while the attach flow runs in the server process, so the server published the offer command and counted the publish as delivery — and the run event loop, the only thing the user is watching, had no handler for it. Neither default nor JSON mode showed anything. The loop now renders it, on stderr for the same reason the local notice is there.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 68037195bb
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const deadline = AbortSignal.timeout(INSTALL_TIMEOUT_MS) | ||
| try { | ||
| const result = await Process.run([npm, "i", "-g", spec], { abort: deadline, nothrow: true }) | ||
| if (result.code === 0) return { ok: true } |
There was a problem hiding this comment.
When npm uses a custom global prefix whose binary directory is not on PATH, the install can exit with code 0 even though the next attach's which(ENGINE_BINARY) still returns null. This branch reports success and the dialog promises that integrations will attach on the next message, but the retry simply returns engine-missing and raises the offer again. Re-run the same binary/version discovery after npm completes and report a PATH-specific failure unless the installed engine is usable.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in the head above — and this is one my own E2E structurally could not have caught, which is worth saying.
Every install run I have done put the isolated npm prefix bin directory on PATH, so the installed engine was always discoverable and the success path always looked right. Your case is the one where it is not.
Reproduced it directly by running the same scenario with the prefix bin deliberately absent from PATH: npm exits 0, datamate --version in the prefix reports 0.7.0, and before the change the dialog said "Workspace engine installed. Integration tools attach on your next message" — a promise nothing could keep, followed by the offer being raised again on the retry.
Fixed by re-running the attach's own discovery after a zero exit: the binary must be findable and clear the version floor, otherwise the install reports why. It now reads "npm installed it, but datamate is not on PATH — add your npm global bin directory to PATH", verified in the same scenario.
Two unit tests added for the shape: zero exit with nothing on PATH, and zero exit with a below-floor engine on PATH.
| let installing = false | ||
|
|
||
| const command = () => props.offer.command | ||
| const canInstall = () => props.nodeMajor !== null && props.nodeMajor >= MIN_NODE_MAJOR |
There was a problem hiding this comment.
When Node 20+ is present but npm is not installed or not on PATH—for example, Linux distributions where nodejs and npm are separate packages—this predicate enables Install now, but installEngine() immediately fails with ENOENT. The recovery Copy command is unusable for the same reason, while the dialog only reports that Node was found; include npm availability in this gate so the UI can explain the actual missing prerequisite.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in the same head.
The gate checked only the Node major, which is not sufficient — Debian and Ubuntu ship nodejs and npm as separate packages, so Node 20+ with no npm is an ordinary state, and there Install now would have appeared and then failed immediately with ENOENT.
Install now now requires npm to be resolvable as well, and the dialog says which prerequisite is actually missing rather than only reporting on Node: "needs npm to install — npm was not found on PATH" when Node is fine but npm is absent, and the existing Node wording otherwise.
One correction to the finding: the recovery Copy command is not affected. It only writes the command to the clipboard for the user to run elsewhere, and does not invoke npm — so it stays useful precisely in this case, which is the point of having it.
npm installs into its configured global prefix, whose bin directory need not be on PATH — so a zero exit could leave the next attach finding nothing while the dialog promised tools on the next message and the offer was raised again. The install now re-runs the attach's own discovery and reports the PATH reason instead. Install now also requires npm, not just Node 20+: several distros package them separately.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 33d37f3777
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if ( | ||
| event.type === "tui.command.execute" && | ||
| (event.properties as { command?: string }).command === WorkspaceEngine.OFFER_COMMAND |
There was a problem hiding this comment.
When the initial attach exceeds ATTACH_WAIT_MS, or a repair retry runs without waiting, the session can reach idle before the server publishes this event; the loop breaks at the preceding idle check and never executes this handler, so run --attach exits without the install notice. Fresh evidence beyond the previously fixed missing-handler issue is that the new handler is placed after the idle break even though engine-sync.ts explicitly allows attach work to continue after the wait expires. Handle or await the pending offer before terminating the event stream.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in fe77f09 — partially, and I want to be exact about which part.
You are right about the placement. I verified the ordering in source rather than assuming: the idle check breaks at the session.status handler, and my round-8 handler was added below it, so an offer arriving in the same batch as idle was never reached. Moved above the break.
What that does NOT fix, and is now noted in place: an offer published strictly after the session goes idle still cannot be rendered there, because the event stream is finished by then. That is a genuine gap when the attach exceeds its bounded wait and finishes late. Closing it properly means either awaiting a pending offer before terminating the stream — which needs the client to know one is coming, and it does not — or a grace window after idle, which would delay the exit of every run for a case that is rare. Neither seemed worth it against a notice; I would rather leave the residual visible than add a timing hack to the exit path of every headless run.
Both other surfaces are unaffected: the TUI shows the dialog, and a local headless run prints during the turn rather than through this loop. Regression-checked the local path after moving the handler — still exactly one line on stderr, no duplicate.
The handler sat after the loop's idle check, so an offer arriving in the same batch as idle was never rendered — the loop had already stopped. Moved above it. An offer published strictly after idle still cannot be shown there, since the stream is over; that residual is noted in place and affects only the attached-run surface.
|
@codex review |
|
Codex Review: Didn't find any major issues. More of your lovely PRs please. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Two tests asserted a stub's own return value and would have passed with the function they named deleted; one asserted only that a constant is positive. The TUI routing invariant was covered twice — the real publish path stays. Every survivor was mutation-checked: printing to stdout, dropping the abort signal, trusting npm's exit code, removing the headless branch and never publishing each turn at least one test red.
e6bfc5d to
78288ed
Compare
|
@codex review |
|
Codex Review: Didn't find any major issues. Already looking forward to the next diff. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
1 participant
Issue for this PR
Closes #1157
Type of change
What does this PR do?
A bound workspace whose declared tools need the local engine used to report a missing engine as a 10-second toast with a command in it. This replaces that with an offer: Install now / Copy command / Not now. The install only ever runs from an explicit choice — attach still never installs anything on its own.
The part worth reviewing is how the offer reaches the TUI. I first registered a handler with the attach module from the plugin. It typechecked, unit-tested green, and did nothing in a real TUI — the user got the old toast, never the dialog. The plugin runtime loads plugins in a separate module realm, so the instance the plugin imports is not the one attach consults; a globalThis key failed likewise. The offer is therefore published on the event bus, which is what toasts already use, and since that event carries no payload the plugin re-derives the detail itself.
Deliberate details:
attach <url>the plugin runs on the client while the workspace is the server's, so the offer refuses to act there and points at the server instead; an attachedrunrenders the notice in its own event loop.How did you verify your code works?
Unit 309 pass / 0 fail on the workspace and plugin suites; full suite green apart from one failure already red on the base. Typecheck clean, lint at baseline on every file touched.
E2E against a real bound workspace with the engine absent from PATH: dialog with all three options and a live declared count; Install now installs into an isolated prefix and tools appear on the next message in the same session; Not now suppresses, and returns once the latch is aged past 7 days; no Node hides Install now; headless prints exactly one line across 3 identical runs.
Not verified: the successful clipboard path — this host has no clipboard backend, so only the "could not confirm" branch ran. Not verified: Windows — the npm shim fix follows existing precedent in this repo rather than a test, and deserves a check before release.
Round-by-round review log (five rounds, seven findings — six fixed here, one routed to the PR below; three were regressions from my own earlier fixes), the second capture, and detailed evidence: see the "Codex review log" comment on this PR.
Screenshots / recordings
Terminal UI, so this is captured pane output rather than an image. Workspace name redacted.
Checklist