feat(review-tutor): host-independent review-tutor CLI - #83
Merged
Conversation
The extension keeps its argv, notifications and forbidden-API boundary while argument parsing, process execution, repository resolution, browser opening and the server lifecycle move to src/cli-support.ts so a CLI can share them. The skill path now resolves from the package root for both TS source and dist, and connector discovery gains one home for its bounds and scrubbed environment.
`review-tutor [source] [--no-open] [--detach] [--home <dir>]` starts the same local server from any shell. Pi now discovers itself through `pi --version` and `pi --list-models` when no host supplies a model snapshot, under the same bounds and scrubbed environment the Codex connector uses. The URL is the only line on stdout, discovery goes to stderr, and `--detach` hands the server to a child that reports its URL once and exits after 30 idle minutes or on SIGTERM. Refs #82
5 tasks
The npm bin is a symlink, so the argv[1] guard in src/cli.ts never matched and the installed command did nothing; a dedicated src/bin.ts entry now owns the shebang and the invocation. Under Pi, repository resolution and browser opening go back to pi.exec with the 5 s and 10 s timeouts through an optional timeoutMs on the ExecFile seam. The foreground arms its signal wait before opening the browser, the idle window restarts for a connection that came and went between two polls, and a failed detach handshake destroys the pipe and unrefs the child. Refs #82
Member
Author
|
Ready for human review. CI green at 40cbc82 (run 33007070051). Last two commits are test-only: the CLI test barrier now yields to the real event loop and is deadline-based (5 s) — the attempt-counted version flaked on a loaded runner while |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What changed
review-tutor <source>now runs from any shell with no Pi host, while the Pi extension keeps its in-process path and its host execution.src/cli-support.tsholds the host-neutral helpers the extension used to own:sourceFromArgument,createExecFileAdapter,resolveRepository,openInBrowser,createServerLifecycle. The extension imports them and re-exports the two names its tests use; its argv, notifications and forbidden-API boundary are unchanged.ExecFilegained an optionaltimeoutMs, soresolveRepository(5 s) andopenInBrowser(10 s) keep the exact timeouts they had, whichever engine runs them. Under Pi both go throughpiExecFile(pi)— anExecFileoverpi.exec— so the extension still uses the host's exec, notnode:child_process. The git/PR inputs path keeps the node adapter it used before this PR.paths.tsexportsdefaultSkillPath(), resolved fromimport.meta.url, so the shipped rubric is found from TS source (src/) and from the builtdist/bin.jsalike.DiscoveryDeps.piModelsis optional. Without it, the Pi connector discovers itself throughpi --versionandpi --list-modelson the sameDiscoveryExecFileseam Codex uses, with the same 10 s / 1 MiB bounds and a scrubbed environment (PATH HOME USER LOGNAME LANG LC_ALL XDG_CONFIG_HOME). With it, behaviour is exactly today's: no spawn,available: true.src/connectors/discovery.tsis the one home for those bounds and the environment allowlist; Codex now uses it too and keeps itsCODEX_HOMEkey.src/cli.tsholds the CLI as importable functions;src/bin.tsis the shebang entry that runs it, published asbin: { "review-tutor": "dist/bin.js" }. Foreground resolves cwd and repo, starts the server, prints the URL as the only stdout line, opens a browser unless--no-open, and stays up until SIGINT/SIGTERM — the signal wait is armed before the browser call, so a signal during a slowxdg-openstill closes the server once and exits 0.--detachspawnsprocess.execPathwith--serve-detached, waits up to 30 s for the child's URL line, unrefs it and returns; a failed handshake SIGTERMs the child, destroys the pipe and unrefs before failing, so the parent cannot be held past the bound.IDLE_EXIT_MS, polled every 60 s) or on SIGTERM.SseHubexposesclientCountand a monotonicconnectionGeneration;IdleTrackerrestarts the window when either shows traffic, so a page that connects and leaves between two polls cannot be missed. The existingReviewTutorServershape is unchanged;StartedReviewTutorServercarries the two counters.bun run buildnow builds this package as the last step.Protocol, page, security headers, token flow, runner limits, existing connectors' spawn args and persistence formats are untouched.
Tested
npx vitest run packages/review-tutor/test— 384 passed (12 files), includingtest/cli.test.ts(argument table, foreground URL/summary/SIGTERM, signal during a pending browser open, detach handshake, handshake timeout with pipe/unref/kill pinned, child exit, idle window, transient-connection window restart) andtest/extension-host-exec.test.ts, which pinspi.exec("git", ["rev-parse","--show-toplevel"], { cwd, timeout: 5000 })andpi.exec("xdg-open", [url], { cwd, timeout: 10000 })withnode:child_processmocked to throw.cd packages/review-tutor && npm run typecheck— clean.bun run check(typecheck, lint, test, coverage, build) — exit 0; 675 passed / 25 skipped, coverage thresholds met.pi/claude/codex/ghshims first onPATHthat log every call, the whole package suite passes and the shim log stays empty — no test spawns a harness.Not tested
open,cmd /c start) stay code-only, as before.npm publish— the package staysprivateat0.0.0in this PR; the bin was proven from a packed tarball instead.Risk class
Serious backend.
Local proof
Packed and installed, then run through the npm bin symlink (tokens redacted):
Earlier run from the built worktree, same shape, plus detach:
Real
pi --list-modelsran there, as the plan allows;claudeandcodexwere only version-probed by discovery.Unspecified choices
--help/-hadded (usage on stdout, exit 0) — implied by the contract's--help-style proof; not in the flag list.createConnectorRegistry({}), not{ execFile }: injecting one discoveryexecFilewould apply it to every connector and flatten the per-connector environment allowlists (CODEX_HOME,XDG_CONFIG_HOME).src/connectors/discovery.tsis a new shared home for the discovery bounds, the scrubbed environment and the exec factory, rather than duplicating them inpi.ts.StartedReviewTutorServer extends ReviewTutorServerinstead of widening the existing interface, so existing server fakes keep compiling.tsup src/bin.ts …without--dts false(tsup would readfalseas an entry); the shebang lives at the top ofsrc/bin.tsand esbuild preserves it.--version→ "Pi could not report a supported version."; missing header or zero usable rows → "Pi could not list its models."provider: model.1 model/N models.--home <dir>maps toServerOptions.home(state root, validated byresolveStatePaths); a value starting with-is a usage error.--no-open, because the parent forwards the original args and owns the open.openInBrowserpassescwd: process.cwd()(theExecFileseam requires a cwd); under Pi that reachespi.execas an explicit cwd where the old call inherited it.createConnectorRegistryin the suite injectswhich/execFilestubs, including the pre-existing helper inconnectors.test.ts.Refs #82