Skip to content

feat(review-tutor): host-independent review-tutor CLI - #83

Merged
ElbertePlinio merged 6 commits into
mainfrom
feat/review-tutor-cli
Aug 26, 2026
Merged

feat(review-tutor): host-independent review-tutor CLI#83
ElbertePlinio merged 6 commits into
mainfrom
feat/review-tutor-cli

Conversation

@ElbertePlinio

@ElbertePlinio ElbertePlinio commented Aug 26, 2026

Copy link
Copy Markdown
Member

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.ts holds 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.
  • ExecFile gained an optional timeoutMs, so resolveRepository (5 s) and openInBrowser (10 s) keep the exact timeouts they had, whichever engine runs them. Under Pi both go through piExecFile(pi) — an ExecFile over pi.exec — so the extension still uses the host's exec, not node:child_process. The git/PR inputs path keeps the node adapter it used before this PR.
  • paths.ts exports defaultSkillPath(), resolved from import.meta.url, so the shipped rubric is found from TS source (src/) and from the built dist/bin.js alike.
  • DiscoveryDeps.piModels is optional. Without it, the Pi connector discovers itself through pi --version and pi --list-models on the same DiscoveryExecFile seam 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.ts is the one home for those bounds and the environment allowlist; Codex now uses it too and keeps its CODEX_HOME key.
  • src/cli.ts holds the CLI as importable functions; src/bin.ts is the shebang entry that runs it, published as bin: { "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 slow xdg-open still closes the server once and exits 0. --detach spawns process.execPath with --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.
  • The detached server exits by itself after 30 minutes with no SSE client (IDLE_EXIT_MS, polled every 60 s) or on SIGTERM. SseHub exposes clientCount and a monotonic connectionGeneration; IdleTracker restarts the window when either shows traffic, so a page that connects and leaves between two polls cannot be missed. The existing ReviewTutorServer shape is unchanged; StartedReviewTutorServer carries the two counters.
  • Root bun run build now 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), including test/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) and test/extension-host-exec.test.ts, which pins pi.exec("git", ["rev-parse","--show-toplevel"], { cwd, timeout: 5000 }) and pi.exec("xdg-open", [url], { cwd, timeout: 10000 }) with node:child_process mocked to throw.
  • cd packages/review-tutor && npm run typecheck — clean.
  • Root bun run check (typecheck, lint, test, coverage, build) — exit 0; 675 passed / 25 skipped, coverage thresholds met.
  • PATH-shim check: with fake pi/claude/codex/gh shims first on PATH that log every call, the whole package suite passes and the shim log stays empty — no test spawns a harness.

Not tested

  • macOS and Windows browser-open paths (open, cmd /c start) stay code-only, as before.
  • npm publish — the package stays private at 0.0.0 in this PR; the bin was proven from a packed tarball instead.
  • No question was asked through a real harness, so the CLI's ask path is covered only by the existing server suites.

Risk class

Serious backend.

Local proof

Packed and installed, then run through the npm bin symlink (tokens redacted):

$ npm pack -> pickforge-review-tutor-0.0.0.tgz ; npm install <tarball> in a temp dir
node_modules/.bin/review-tutor -> ../@pickforge/review-tutor/dist/bin.js

$ node_modules/.bin/review-tutor --help
Usage: review-tutor [source] [--no-open] [--detach] [--home <dir>]
  source     worktree (default), staged, <revision>, <from>...<to>, or a GitHub PR URL
  --no-open  print the URL without opening a browser
  --detach   leave the server running in the background and return immediately
  --home     store Review Tutor state under this absolute directory
help exit=0

$ node_modules/.bin/review-tutor worktree --no-open --home <tmp>   # in a temp git repo
stdout: http://127.0.0.1:46515/?session=<redacted>     (1 line)
stderr: Pi: 434 models / Claude Code: 3 models / Codex: 7 models
GET /api/state (Bearer <token>): pi available 434 models, claude-code available 3, codex available 7
  input: "Local worktree diff" 126 bytes
SIGTERM -> exit=0

Earlier run from the built worktree, same shape, plus detach:

foreground: one URL line, all three harnesses available, worktree diff 25091 bytes, SIGTERM -> exit 0
--detach:   parent exit=0 after 1s, one URL line, /api/state -> 200,
            detached pid listening on 127.0.0.1:36363, SIGTERM -> process gone, connection refused

Real pi --list-models ran there, as the plan allows; claude and codex were only version-probed by discovery.

Unspecified choices

  1. --help / -h added (usage on stdout, exit 0) — implied by the contract's --help-style proof; not in the flag list.
  2. Exit codes: 0 success, 1 runtime failure, 2 usage.
  3. CLI builds the registry as createConnectorRegistry({}), not { execFile }: injecting one discovery execFile would apply it to every connector and flatten the per-connector environment allowlists (CODEX_HOME, XDG_CONFIG_HOME).
  4. src/connectors/discovery.ts is a new shared home for the discovery bounds, the scrubbed environment and the exec factory, rather than duplicating them in pi.ts.
  5. StartedReviewTutorServer extends ReviewTutorServer instead of widening the existing interface, so existing server fakes keep compiling.
  6. Build: tsup src/bin.ts … without --dts false (tsup would read false as an entry); the shebang lives at the top of src/bin.ts and esbuild preserves it.
  7. Pi failure texts mirror Codex: unparsable or failed --version → "Pi could not report a supported version."; missing header or zero usable rows → "Pi could not list its models."
  8. Table parsing requires all six header column names and at least six columns per row; the label is provider: model.
  9. Discovery is memoized in the CLI so the stderr summary and the server share one probe pass; summary pluralizes 1 model / N models.
  10. --home <dir> maps to ServerOptions.home (state root, validated by resolveStatePaths); a value starting with - is a usage error.
  11. The detached child never opens a browser, even without --no-open, because the parent forwards the original args and owns the open.
  12. openInBrowser passes cwd: process.cwd() (the ExecFile seam requires a cwd); under Pi that reaches pi.exec as an explicit cwd where the old call inherited it.
  13. Test hygiene: every createConnectorRegistry in the suite injects which/execFile stubs, including the pre-existing helper in connectors.test.ts.

Refs #82

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
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
@ElbertePlinio

Copy link
Copy Markdown
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 realpath waited in the fs thread pool. Review panel (Sol correctness, Grok security, Opus test integrity) was clean on the prior head; the change since is confined to test/cli.test.ts.

@ElbertePlinio
ElbertePlinio merged commit df0c20a into main Aug 26, 2026
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant