feat(vscode-ext): add VS Code extension that launches the Raven TUI - #373
feat(vscode-ext): add VS Code extension that launches the Raven TUI#373LivXue wants to merge 15 commits into
Conversation
Add lint/test/build targets for vscode-ext to the Makefile, eslint and prettier pre-commit hooks, and a CI job running lint, type-check, test, and build on Node 22.
Scaffold vscode-ext, a VS Code extension that opens the Raven TUI in a dedicated integrated terminal, mirroring the OpenCode extension model. - raven.openTui command plus a status bar shortcut - raven resolution: raven.executablePath or RAVEN_BIN (~ expanded), then command -v / where, then a bash/zsh login-shell probe, then uv run raven - single-terminal reuse: re-show while alive, recreate after close - posix and win32 shell quoting for the launch line - narrow vscode facade keeps the launcher logic unit-testable (16 tests) - .vscode launch config for F5 extension-host debugging
- probe PATH with /usr/bin/which instead of the command builtin name, which never spawned (ENOENT) and forced the slow login-shell fallback on every lookup; on minimal environments without bash/zsh this misreported raven as missing - quote launch-line args with a safe-character whitelist so shell metacharacters (& ; | < > etc.) and empty args survive on posix and win32 - move PATH probing into pathProbe.ts with an injected runner for unit tests - reject directories for raven.executablePath / RAVEN_BIN (stat isFile) - expand Windows-style ~\ prefixes and drop the unused platform field - fix stale vitest.config.ts reference in .vscodeignore
- fix the package license to Apache-2.0 to match the repository - add repository metadata, keywords, README, and LICENSE so vsce can package - add @vscode/vsce with package and publish scripts - ignore built .vsix artifacts
gloryfromca
left a comment
gloryfromca
left a comment
There was a problem hiding this comment.
Blocking: the launcher cannot reopen Raven after a normal TUI exit, Windows arguments remain shell-expandable, and several new code files violate the repository's required file-level documentation rule; see the inline notes.
Coverage: I reviewed the current github/main...HEAD diff, AGENTS.md, CLAUDE.md, CONTEXT-MAP.md, the TUI glossary, the extension entry point and internal callers, the VS Code terminal lifecycle contract, the branch history, backward compatibility, and all test changes. The shared Makefile, pre-commit, and CI changes are additive, and no existing tests were weakened.
Verification:
npm ci --prefix vscode-ext: passed.npm run lint,npm run type-check,npm test,npm run build, andnpm run fmtinvscode-ext: passed; Vitest reported 3 files and 27 tests passed.npm audit --prefix vscode-ext --audit-level=critical: passed with 0 vulnerabilities.npm run package --prefix vscode-ext -- --no-dependencies: passed; VSIX contained 11 files and was 11.28 KB.scripts/check_large_files.py github/main...HEAD, commitlint, the repository commit-message checker, andgit diff --check: passed.
make is not installed in this environment, so I ran its extension targets directly and invoked the large-file checker with the exact uv run command used by the Make target.
| } | ||
|
|
||
| open(command: RavenCommand, workspacePath: string | undefined, platform: NodeJS.Platform): void { | ||
| if (this.terminal) { |
There was a problem hiding this comment.
[P1] Track Raven execution, not terminal existence
onDidCloseTerminal fires when the terminal is disposed, but this terminal runs an ordinary shell and receives raven tui through sendText. A normal Raven quit returns to that shell without closing the terminal, so this.terminal stays non-null. Every later Raven: Open TUI invocation therefore takes this branch and only reveals an idle prompt; it never launches Raven again. Please make the terminal process be Raven itself (for example via shellPath/shellArgs) or track command completion and reset the running state.
There was a problem hiding this comment.
Fixed in ad9bacc: the terminal process is now raven itself (shellPath/shellArgs), so a normal TUI exit closes the terminal, onDidCloseTerminal resets the manager, and the next Raven: Open TUI recreates it. Covered by the "recreates the terminal after it closes" test.
| export function buildSendText(command: RavenCommand, platform: NodeJS.Platform): string { | ||
| const argv = [command.command, ...command.args] | ||
| if (platform === 'win32') { | ||
| return argv.map(arg => (WIN32_SAFE_ARG.test(arg) ? arg : `"${arg.replaceAll('"', '""')}"`)).join(' ') |
There was a problem hiding this comment.
[P1] Avoid host-shell expansion on Windows
win32 does not identify the user's shell: the integrated terminal may be PowerShell or cmd. For example, an extra argument $(Get-ChildItem) is emitted as "$(Get-ChildItem)", which PowerShell evaluates, while %PATH% is expanded by cmd even inside double quotes. Those values are not passed literally, and configured arguments can execute as shell syntax despite the claimed shell-safe quoting. Please launch with an argv-preserving API/process rather than sendText, or otherwise handle the actual shell without leaving expansion forms active.
There was a problem hiding this comment.
Fixed in ad9bacc: launch switched from sendText to argv-preserving shellPath/shellArgs, so arguments reach raven directly with no host-shell expansion on any platform. buildSendText and its quoting tests were dropped entirely.
| @@ -0,0 +1,71 @@ | |||
| import { describe, expect, it, vi } from 'vitest' | |||
There was a problem hiding this comment.
[P1] Add the required file-level purpose documentation
AGENTS.md section 1.1 requires every new code file to document its purpose in English. This test starts directly with imports, as do ravenBin.test.ts, terminal.test.ts, eslint.config.mjs, and vitest.config.mts. Because AGENTS.md defines these as hard constraints, please add appropriate file-level documentation to each new module before merging.
There was a problem hiding this comment.
Fixed in ad9bacc: file-level purpose documentation added to ravenBin.test.ts, terminal.test.ts, pathProbe.test.ts, eslint.config.mjs, and vitest.config.mts per AGENTS.md section 1.1.
…iles Addresses PR #373 review findings: - launch raven via terminal shellPath/shellArgs so the terminal process is raven itself: a normal TUI exit closes the terminal and the next Raven: Open TUI recreates it, and arguments reach raven through argv without host-shell expansion on any platform (drops buildSendText) - add the required file-level purpose documentation to ravenBin.test.ts, terminal.test.ts, pathProbe.test.ts, eslint.config.mjs, and vitest.config.mts - update the README feature list to match the argv-based launch
gloryfromca
left a comment
gloryfromca
left a comment
There was a problem hiding this comment.
No blockers; this can merge as far as I am concerned.
The new commit resolves all three prior findings: Raven is now the terminal process via shellPath/shellArgs, so a normal process exit resets the terminal lifecycle and arguments no longer pass through a host shell; all previously identified test and config modules now have the required file-level purpose documentation.
Coverage: I reviewed the delta from 2db98a5d43f6, rechecked the full current github/main...HEAD diff, repository rules and domain context, the extension entry point and terminal API adapter, branch history, backward compatibility, and test changes. The removed tests only exercised the deleted buildSendText implementation; replacement tests cover direct Raven and uv argv delivery, while reuse, close/recreate, and disposal coverage remains. I found no test weakening intended to manufacture a green result.
Verification:
npm ci --prefix vscode-ext: passed with localEBADENGINEwarnings because Node v23.10.0 is outside two dependencies' supported even-major ranges; CI uses Node 22.- Extension lint, type-check, test, build, and formatting checks: passed; Vitest reported 3 files and 23 tests passed.
npm audit --prefix vscode-ext --audit-level=critical: passed with 0 vulnerabilities.- VSIX packaging: passed; 11 files, 11.15 KB.
- Large-file check, commitlint, repository commit-message checker, and
git diff --check: passed.
make remains unavailable in this environment, so I ran the underlying extension and repository commands directly.
|
Round 1 findings (three P1) are fixed in ad9bacc: argv-preserving launch so a TUI exit reopens correctly and no shell expansion occurs, plus file-level docs on the five new files. PR description updated with the round 1 note; all gates pass (23 tests). |
Show the raven.openTui entry as an editor title button (menus.editor/title, navigation group) next to the active file tab, like the OpenCode and Claude Code extensions, and drop the status bar item, its facade surface, and the onStartupFinished activation.
gloryfromca
left a comment
gloryfromca
left a comment
There was a problem hiding this comment.
Blocking: the code delta is sound, but the PR description must be corrected before merge because it contradicts this revision and becomes the squash commit body.
The description still says the extension uses whitelist-based POSIX/Windows shell quoting, reports 27 tests, records a real-shell quoting smoke test, and says the launch line is shell-quoted. This head removed buildSendText and its quoting tests in favor of direct shellPath/shellArgs; the current suite has 23 tests. The appended round-1 note states the new design, but it leaves the Summary, Verification, and Risk sections internally contradictory. AGENTS.md section 3.7 requires those sections to describe the current overall implementation and exact verification that will land in the squash commit.
Please replace the obsolete shell-quoting and 27-test claims with the direct-argv design and current 23-test/package results. No code change is required for this finding.
Coverage: I reviewed the delta from ad9bacccd803, the current full github/main...HEAD diff, repository rules and domain context, the command contribution and activation path, API facade callers, history, backward compatibility, and test changes. Moving the shortcut from the status bar to menus.editor/title is consistent across the manifest, implementation, facade, tests, and README. The removed test scaffolding only represented the deleted status-bar API; terminal behavior coverage remains, so tests were not weakened to obtain a green result.
Verification:
npm ci --prefix vscode-ext: passed with localEBADENGINEwarnings because Node v23.10.0 is outside two dependencies' supported even-major ranges; CI uses Node 22.- Extension lint, type-check, test, build, and formatting checks: passed; Vitest reported 3 files and 23 tests passed.
npm audit --prefix vscode-ext --audit-level=critical: passed with 0 vulnerabilities.- VSIX packaging: passed; 11 files, 10.93 KB.
- Large-file check, commitlint, repository commit-message checker, and
git diff --check: passed.
make is unavailable in this environment, so I ran the underlying targets directly.
Create the Raven terminal with location: editor so the TUI opens next to the active file tab like the OpenCode extension, instead of the terminal panel. The adapter maps the facade's location flag to viewColumn Beside.
gloryfromca
left a comment
gloryfromca
left a comment
There was a problem hiding this comment.
Blocking: the previously reported PR-description blocker remains unresolved; I found no new code finding in this revision.
The c5ca365 delta correctly maps the facade's editor location to ViewColumn.Beside, passes that placement from the terminal manager, and updates its tests and README consistently. I rechecked the affected callers, current full diff, repository rules and domain context, history, backward compatibility, and test changes. Existing lifecycle and argv coverage remains intact, so the tests were not weakened to obtain a green result.
Verification:
npm ci --prefix vscode-ext: passed with the same localEBADENGINEwarnings from Node v23.10.0; CI uses Node 22.- Extension lint, type-check, test, build, and formatting checks: passed; Vitest reported 3 files and 23 tests passed.
npm audit --prefix vscode-ext --audit-level=critical: passed with 0 vulnerabilities.- VSIX packaging: passed; 11 files, 11.11 KB.
- Large-file check, commitlint, repository commit-message checker, and
git diff --check: passed.
make is unavailable in this environment, so I ran the underlying targets directly.
gloryfromca
left a comment
gloryfromca
left a comment
There was a problem hiding this comment.
Blocking: the corrected PR description resolves the prior blocker, but the 0.1.1 bump leaves the committed lockfile on 0.1.0; see the inline note.
Coverage: I reviewed the delta from c5ca36550a4d, the live PR description, the current full github/main...HEAD diff, repository rules and domain context, package metadata and packaging output, affected callers, history, backward compatibility, and test changes. The PR description now accurately reflects the direct-argv implementation and 23-test suite. No tests were weakened.
Verification:
npm ci --prefix vscode-ext: passed with localEBADENGINEwarnings because Node v23.10.0 is outside two dependencies' supported even-major ranges; CI uses Node 22.- Extension lint, type-check, test, build, and formatting checks: passed; Vitest reported 3 files and 23 tests passed.
npm audit --prefix vscode-ext --audit-level=critical: passed with 0 vulnerabilities.- VSIX packaging: passed and produced
raven-vscode-0.1.1.vsix; 11 files, 11.12 KB. - Large-file check, commitlint, repository commit-message checker, and
git diff --check: passed.
make is unavailable in this environment, so I ran the underlying targets directly.
| "name": "raven-vscode", | ||
| "displayName": "Raven Agent", | ||
| "description": "Open the Raven TUI inside the VS Code integrated terminal.", | ||
| "version": "0.1.1", |
There was a problem hiding this comment.
[P1] Keep the lockfile version in sync
This changes the package to 0.1.1, but both the top-level version and packages[""].version in package-lock.json remain 0.1.0. The packaged VSIX is therefore identified as 0.1.1 while the committed lock metadata still identifies the root package as 0.1.0. npm ci does not catch this root-version drift (it passed here), so please regenerate the lockfile through npm so all committed release metadata agrees on 0.1.1.
There was a problem hiding this comment.
Fixed in 48fd061: the lockfile was regenerated through npm, so the top-level version and packages[""].version now match 0.1.1 in package-lock.json.
Regenerate package-lock.json through npm so the committed root version matches the package.json bump. Co-authored-by: Claude <claude-sonnet-4-6> <noreply@anthropic.com>
gloryfromca
left a comment
gloryfromca
left a comment
There was a problem hiding this comment.
No blockers; this can merge as far as I am concerned.
The prior lockfile finding is resolved: package.json, package-lock.json's top-level version, and packages[""].version all read 0.1.1. The new commit changes only those two lockfile metadata fields, with no dependency or integrity churn. The live PR description also accurately records the fix and remains ASCII-only.
Coverage: I reviewed the delta from 176c5a6, rechecked the full current github/main...HEAD diff and live PR description, repository rules (AGENTS.md and CLAUDE.md) and domain context, package metadata, affected callers, branch history, backward compatibility, and test changes. The extension remains additive, and this revision changes no tests, so no tests were weakened to obtain a green result.
Verification:
- npm ci, lint, type-check, test, build, and formatting checks: passed; Vitest reported 3 files and 23 tests passed. npm ci emitted local EBADENGINE warnings because Node v23.10.0 is outside two dependencies' supported even-major ranges; CI uses Node 22.
- npm audit --audit-level=critical: passed with 0 vulnerabilities.
- VSIX packaging: passed; raven-vscode-0.1.1.vsix contained 11 files and was 11.12 KB.
- Large-file check, commitlint, repository commit-message checker, and git diff --check: passed.
- Current GitHub checks are green for the extension, TUI, bridge, Windows self-upgrade, lint, repository files, commit messages, PR title, and pre-commit diff; the unrelated Python unit job was still pending when reviewed.
Probe the login shell environment (bash/zsh on posix, powershell.exe/pwsh on Windows) and merge it into the Raven terminal so proxy settings and API keys from the user's shell rc apply inside VS Code, fixing OpenRouter regional 403s that only appeared when launching raven through the extension. The probe is cached after the first open.
gloryfromca
left a comment
gloryfromca
left a comment
There was a problem hiding this comment.
Blocking: the Windows environment probe skips the user's PowerShell profile, and the new commits have made the PR description stale again; see the inline note and summary below.
The live description still says version 0.1.1, 23 tests, and a 0.1.1 VSIX, while this revision is 0.1.2 with 31 tests. More importantly, its Risk section says the extension handles no credentials, but this head deliberately copies proxy settings and API keys into the Raven terminal. Because the description becomes the squash commit body, AGENTS.md section 3.7 requires the Summary, Verification, and Risk sections to describe this current behavior and exact results. Please update those claims before merge.
Coverage: I reviewed the delta from 48fd061, rechecked the full current github/main...HEAD diff and live PR description, AGENTS.md, CLAUDE.md, CONTEXT-MAP.md and the TUI glossary, the extension entry point, path probing, VS Code API adapter, terminal manager and tests, branch history, backward compatibility, package metadata, and whether tests were weakened. The eight new tests are additive and existing coverage remains, although the Windows test currently asserts the defective -NoProfile invocation rather than validating a profile-defined variable.
Verification:
- npm ci, lint, type-check, test, build, and formatting checks: passed; Vitest reported 3 files and 31 tests passed. npm ci emitted local EBADENGINE warnings because Node v23.10.0 is outside two dependencies' supported even-major ranges; CI uses Node 22.
- npm audit --audit-level=critical: passed with 0 vulnerabilities.
- VSIX packaging: passed; raven-vscode-0.1.2.vsix contained 11 files and was 11.99 KB.
- Large-file check, commitlint, repository commit-message checker, and git diff --check: passed.
- Current GitHub checks are green for the extension, TUI, bridge, Windows self-upgrade, lint, repository files, commit messages, PR title, and pre-commit diff; the unrelated Python unit job was still pending when reviewed.
| const script = | ||
| '[Console]::OutputEncoding=[System.Text.Encoding]::UTF8; Get-ChildItem Env: | ForEach-Object { "$($_.Name)=$($_.Value)" }' | ||
| for (const shell of ['powershell.exe', 'pwsh']) { | ||
| const result = runner(shell, ['-NoProfile', '-NonInteractive', '-Command', script], { |
There was a problem hiding this comment.
[P1] Load the PowerShell profile before reading its environment
-NoProfile explicitly prevents PowerShell from loading $PROFILE. For a GUI-launched VS Code where an API key or proxy variable is assigned in that profile, this subprocess only repeats the environment VS Code already had, so Raven still misses the value and the advertised Windows behavior is ineffective. The test currently locks in this flag instead of exercising a profile-defined variable. Please probe the user's actual profile-loading shell environment and cover that case.
There was a problem hiding this comment.
Fixed in 1e73eef: -NoProfile removed so the probe subprocess loads the user PowerShell profile, keeping -NonInteractive to avoid prompts; timeout raised to 15000 for profile startup cost. The test now asserts the profile-loading invocation.
Drop -NoProfile so the env probe subprocess loads the user's PowerShell profile, where proxy and API key variables live; keep -NonInteractive to avoid prompts and raise the timeout for profile startup cost.
gloryfromca
left a comment
gloryfromca
left a comment
There was a problem hiding this comment.
Blocking: removing -NoProfile fixes the prior finding for Windows PowerShell, but the probe still bypasses PowerShell 7 profiles, and the previously reported PR-description blocker remains unresolved; see the inline note.
The live Summary, Verification, and Risk sections still contain the already-reported 0.1.1, 23-test, and no-credentials claims. Appending a round note does not correct the current-state sections that become the squash commit body, so that blocker remains.
Coverage: I reviewed the delta from f2a30d8, rechecked the full current github/main...HEAD diff and live PR description, AGENTS.md, CLAUDE.md, CONTEXT-MAP.md and the TUI glossary, path probing and its callers, the VS Code API adapter, terminal lifecycle, branch history, backward compatibility, package metadata, and test changes. The modified test accurately verifies removal of -NoProfile, and no existing test was weakened, but it does not cover selecting the user's actual PowerShell edition/profile.
Verification:
- npm ci, lint, type-check, test, build, and formatting checks: passed; Vitest reported 3 files and 31 tests passed. npm ci emitted local EBADENGINE warnings because Node v23.10.0 is outside two dependencies' supported even-major ranges; CI uses Node 22.
- npm audit --audit-level=critical: passed with 0 vulnerabilities.
- VSIX packaging: passed; raven-vscode-0.1.2.vsix contained 11 files and was 11.97 KB.
- Large-file check, commitlint, repository commit-message checker, and git diff --check: passed.
- Current GitHub checks are green for the extension, TUI, bridge, Windows self-upgrade, lint, repository files, commit messages, PR title, and pre-commit diff; the unrelated Python unit job was still pending when reviewed.
| function probeWindowsEnv(runner: ProbeRunner): Record<string, string> | null { | ||
| const script = | ||
| '[Console]::OutputEncoding=[System.Text.Encoding]::UTF8; Get-ChildItem Env: | ForEach-Object { "$($_.Name)=$($_.Value)" }' | ||
| for (const shell of ['powershell.exe', 'pwsh']) { |
There was a problem hiding this comment.
[P1] Select the user's actual PowerShell profile
On a normal Windows host, powershell.exe is present and this command returns a nonempty environment, so the loop exits before ever trying pwsh. A user who has PowerShell 7 configured in VS Code and defines the proxy or API key in that edition's distinct $PROFILE still gets the Windows PowerShell profile instead, leaving Raven without the value. Please derive the configured/default shell (or otherwise select the correct edition) and cover the PowerShell 7 profile case.
There was a problem hiding this comment.
Fixed in 466e9aa: the Windows env probe now derives the VS Code-configured shell from terminal.integrated.defaultProfile.windows / profiles.windows, probes that edition first, and merges powershell.exe and pwsh environments with the preferred edition winning conflicts. Tests cover the PowerShell 7 profile case.
…ofiles Derive the user's Windows shell from terminal.integrated.defaultProfile.windows and probe that edition first, then merge the powershell.exe and pwsh environments with the preferred edition winning conflicts, so profile-defined variables from PowerShell 7 reach raven even when Windows PowerShell also succeeds.
gloryfromca
left a comment
gloryfromca
left a comment
There was a problem hiding this comment.
Blocking: explicit path-based PowerShell 7 profiles now win correctly, but source-based and auto-detected VS Code profiles still lose their edition hint, and the previously reported PR-description blocker remains unresolved; see the inline note.
The live Summary, Verification, and Risk sections still contain the already-reported 0.1.1, 23-test, and no-credentials claims. Appending another review-round note does not correct those current-state sections, which become the squash commit body.
Coverage: I reviewed the delta from 1e73eef, rechecked the full current github/main...HEAD diff and live PR description, AGENTS.md, CLAUDE.md, CONTEXT-MAP.md and the TUI glossary, VS Code's terminal-profile configuration contract, path probing and configuration callers, the API adapter, terminal lifecycle, branch history, backward compatibility, package metadata, and test changes. The merge-precedence tests are additive and existing tests were not weakened, but they inject a literal path directly and do not exercise shellHint's configuration boundary.
Verification:
- npm ci, lint, type-check, test, build, and formatting checks: passed; Vitest reported 3 files and 32 tests passed. npm ci emitted local EBADENGINE warnings because Node v23.10.0 is outside two dependencies' supported even-major ranges; CI uses Node 22.
- npm audit --audit-level=critical: passed with 0 vulnerabilities.
- VSIX packaging: passed; raven-vscode-0.1.2.vsix contained 11 files and was 12.25 KB.
- Large-file check, commitlint, repository commit-message checker, and git diff --check: passed.
- Current GitHub checks are green for the extension, TUI, bridge, Windows self-upgrade, lint, repository files, commit messages, PR title, and pre-commit diff; the unrelated Python unit job was still pending when reviewed.
| const config = api.workspace.getConfiguration('terminal.integrated') | ||
| const defaultProfile = config.get<string>('defaultProfile.windows') | ||
| const profiles = config.get<Record<string, { path?: string }>>('profiles.windows') | ||
| return (defaultProfile && profiles?.[defaultProfile]?.path) || undefined |
There was a problem hiding this comment.
[P1] Resolve source-based and auto-detected profiles
VS Code supports Windows profiles declared as { source: "PowerShell" } and auto-detects default profiles without requiring a literal path (official profile documentation). For those common PowerShell 7 configurations, this expression returns undefined; probeWindowsEnv then uses its generic order and lets powershell.exe win conflicts, so the PowerShell 7 $PROFILE value is still lost. The new test bypasses this boundary by passing a path directly. Please resolve the actual configured/default executable for source-based and auto-detected profiles and cover that configuration path.
There was a problem hiding this comment.
Fixed in 8463ac1: the shell hint now comes from vscode.env.shell, which VS Code itself resolves from terminal.integrated.defaultProfile including source-based and auto-detected profiles; the manual profiles.windows path lookup remains only as a fallback. resolveWindowsShellHint is a pure function with tests covering env.shell preference, path fallback, and source-based profiles without a path.
Use vscode.env.shell, which VS Code resolves from defaultProfile including source-based and auto-detected profiles, as the preferred Windows shell hint; the manual profiles.windows path lookup stays as a fallback. Covers PowerShell 7 setups declared without a literal path.
gloryfromca
left a comment
gloryfromca
left a comment
There was a problem hiding this comment.
Blocking: the source-based/auto-detected profile defect is fixed and I found no new code issue, but this commit has made the PR description's exact verification stale again.
vscode.env.shell is the detected extension-host shell and is overridden by terminal.integrated.defaultProfile, so it supplies the resolved executable that the prior implementation lacked. The fallback remains available when that value is empty, and the new pure-function tests cover its precedence.
The live Verification and Summary still report 32 tests and a 12.25 KB VSIX, while this revision actually has 36 passing tests and packages 11 files at 12.33 KB. Because these current-state sections become the squash commit body and AGENTS.md section 3.7 requires exact commands and results, please update those two figures before merge.
Coverage: I reviewed the delta from 466e9aa, rechecked the full current github/main...HEAD diff and live PR description, AGENTS.md, CLAUDE.md, CONTEXT-MAP.md and the TUI glossary, the VS Code env.shell and terminal-profile contracts, API adapter and configuration callers, path probing, terminal lifecycle, branch history, backward compatibility, package metadata, and test changes. The four added tests are additive and no existing tests were weakened.
Verification:
- npm ci, lint, type-check, test, build, and formatting checks: passed; Vitest reported 3 files and 36 tests passed. npm ci emitted local EBADENGINE warnings because Node v23.10.0 is outside two dependencies' supported even-major ranges; CI uses Node 22.
- npm audit --audit-level=critical: passed with 0 vulnerabilities.
- VSIX packaging: passed; raven-vscode-0.1.2.vsix contained 11 files and was 12.33 KB.
- Large-file check, commitlint, repository commit-message checker, and git diff --check: passed.
- Current GitHub checks are green for the extension, TUI, bridge, Windows self-upgrade, lint, and pre-commit diff; the unrelated Python unit job was still pending when reviewed.
|
Verified against the live description: the Verification and Summary sections now report the current figures (3 files / 36 tests, VSIX 11 files / 12.33 KB). The remaining mentions of 32 tests are inside the historical Round 7 note, which describes the state at that revision. No code change was needed for this round. |
|
Confirmed. The live current-state Summary and Verification now report 3 files / 36 tests and an 11-file / 12.33 KB VSIX. The remaining 32-test text is clearly historical Round 7 context, so it does not contradict this revision. I reran |
gloryfromca
left a comment
gloryfromca
left a comment
There was a problem hiding this comment.
No blockers; this can merge as far as I am concerned.
The live PR description now resolves the only remaining blocker: its current-state Summary and Verification accurately report version 0.1.2, 3 files / 36 tests, and an 11-file / 12.33 KB VSIX, while the 32-test mention is correctly scoped to historical Round 7 context. No code changed since my review of this head, where the vscode.env.shell fix resolved source-based and auto-detected PowerShell profiles without introducing a new code issue.
Verification on this answer turn: npm test --prefix vscode-ext passed all 36 tests, and git diff --check github/main...HEAD passed.
Sanitize the extraArgs setting at the config boundary: non-array values become an empty list and only string entries survive, so a misconfigured string no longer spreads into single-character argv entries. Add sanitizeExtraArgs with unit tests.
gloryfromca
left a comment
gloryfromca
left a comment
There was a problem hiding this comment.
No blockers; this can merge as far as I am concerned.
The new config-boundary sanitizer fixes the reported runtime type hazard: a scalar raven.extraArgs value now becomes an empty list, mixed arrays cannot pass non-string values into terminal argv, and valid string arrays preserve their order and contents. The live PR description accurately reflects this revision's 40-test and 12.41 KB package results.
Coverage: I reviewed the delta from 8463ac1, rechecked the full current github/main...HEAD diff and live PR description, AGENTS.md, CLAUDE.md, CONTEXT-MAP.md and the TUI glossary, the extension configuration boundary, Raven command construction and callers, package schema, branch history, backward compatibility, package metadata, and test changes. The four sanitizer tests are additive and no existing test was weakened.
Verification:
- npm ci, lint, type-check, test, build, and formatting checks: passed; Vitest reported 3 files and 40 tests passed. npm ci emitted local EBADENGINE warnings because Node v23.10.0 is outside two dependencies' supported even-major ranges; CI uses Node 22.
- npm audit --audit-level=critical: passed with 0 vulnerabilities.
- VSIX packaging: passed; raven-vscode-0.1.2.vsix contained 11 files and was 12.41 KB.
- Large-file check, commitlint, repository commit-message checker, and git diff --check: passed.
- Current GitHub checks are green for the extension, TUI, bridge, Windows self-upgrade, lint, repository files, commit messages, PR title, and pre-commit diff; the unrelated Python unit job was still pending when reviewed.
2 participants
Summary
Adds vscode-ext, a VS Code extension that opens the Raven TUI in an editor-area tab next to the active file, mirroring the OpenCode extension model.
Post-review UI updates: the launch button lives in the editor title bar (menus.editor/title) instead of the status bar, the TUI opens as an editor-area tab (viewColumn Beside), and the version is 0.1.2 for marketplace publication.
Type
Verification
npm run lint (pass), npm run type-check (pass), npm run build (pass), npm run fmt (pass), npm test (3 files, 40 tests pass)
npm audit --audit-level=critical (pass, 0 vulnerabilities)
npm run package builds raven-vscode-0.1.2.vsix (11 files, 12.41 KB)
commitlint and scripts/check_commit_messages.py over the branch (pass)
CI on the branch: VS Code extension checks, TUI checks, bridge checks, commit messages, pull request title, repository files, python lint, Windows self-upgrade, and pre-commit diff all pass; the unrelated Python unit job was still pending at review time
Relevant tests pass locally
Relevant lint / type checks pass locally
User-facing docs or screenshots are updated when needed
Risk
No existing behavior changes: the extension is a new top-level package and the repo wiring only adds targets, hooks, and one CI job. Arguments reach raven through argv, so the extension performs no shell quoting. The extension launches raven with the user's login shell environment merged in, which can include proxy settings and API keys defined in that shell's profile; this mirrors what a direct terminal run would have and is the intended behavior. Rollback is deleting vscode-ext/ and reverting the wiring commit.
Related Issues
N/A
Review rounds (gloryfromca):
Round 1 (three P1 findings): fixed in ad9bacc. Raven now launches as the terminal process itself (shellPath/shellArgs), so a normal TUI exit closes the terminal and the next Raven: Open TUI recreates it, and arguments reach raven through argv without host-shell expansion on any platform; buildSendText was dropped with its tests. The five new files missing file-level purpose docs (ravenBin.test.ts, terminal.test.ts, pathProbe.test.ts, eslint.config.mjs, vitest.config.mts) now carry them.
Round 2: no blockers, can merge.
Rounds 3-4 (stale PR description): corrected here; Summary, Verification, and Risk now describe the direct-argv design and the current 23-test suite. LivXue confirmed all three round-1 findings fixed in ad9bacc. No code change was required for this round.
Round 5 (P1 lockfile drift): fixed in 48fd061. package-lock.json still identified the root package as 0.1.0 while package.json read 0.1.1, so the packaged VSIX was 0.1.1 but the committed lock metadata said 0.1.0. The lockfile was regenerated through npm (npm install) so both the top-level version and packages[""].version in package-lock.json now agree on 0.1.1. npm run lint / type-check / test / build / fmt pass.
Review round 6 (P1 PowerShell profile): fixed in 1e73eef. The Windows env probe no longer passes -NoProfile, so powershell.exe loads the user profile before the environment is read; -NonInteractive avoids prompts and the timeout covers profile startup cost.
Review round 7 (P1 PowerShell edition): fixed in 466e9aa. The probe reads terminal.integrated.defaultProfile.windows to select the user's actual PowerShell edition, probes it first, and merges both editions with the preferred one winning, so PowerShell 7 profile variables reach raven.
Round 7 follow-up (stale description): the Summary, Verification, and Risk sections above were corrected to the current 0.1.2 / 32-test / merged-login-env state so the squash commit body matches this revision; gates re-ran on 466e9aa (lint, type-check, test: 3 files / 32 tests, build, fmt, audit, and raven-vscode-0.1.2.vsix packaging).
Review round 8 (P1 source-based / auto-detected profiles): fixed in 8463ac1. The Windows shell hint now comes from vscode.env.shell, which VS Code itself resolves from terminal.integrated.defaultProfile including source-based and auto-detected profiles (no literal path needed); the manual profiles.windows lookup remains only as a fallback inside the pure resolveWindowsShellHint helper, which has direct unit coverage. Gates re-ran on the new head: lint, type-check, test: 3 files / 36 tests, build, fmt, audit, and raven-vscode-0.1.2.vsix packaging; CI extension, TUI, bridge, Windows self-upgrade, pre-commit diff, and python lint checks pass.
LivXue review (two findings): both fixed, verified on the current head.
Gates re-ran after both fixes: npm run lint, type-check, test (3 files, 36 tests), build, and fmt all pass.
Review round 9 (docs bug sweep #7): fixed in bfb1b7d. raven.extraArgs is now sanitized at the config boundary (sanitizeExtraArgs), so a misconfigured string cannot spread into single-character argv entries; only string entries of an array survive. Live figures updated to 40 tests and a 12.41 KB VSIX.