feat(extensions): add generic CLI commands - #888
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
Greptile SummaryThis PR publishes Extension API v10 with generic top-level CLI command trees, raw streaming I/O, cancellation, validated exits, and one-time delegation into built-in commands.
Confidence Score: 4/5The PR appears safe to merge, with one non-blocking repository-convention issue in the GitHub example's environment access. The extension command, delegation, registry-lifecycle, stream-ownership, and exact-file reload paths include coordinated validation and tests; the remaining accepted concern is direct process.env access in the new example. Files Needing Attention: examples/extensions/github-pr/index.ts Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Raw argv] --> B{Built-in command?}
B -- Yes --> C[Existing built-in planner]
B -- No --> D{Valid extension command name?}
D -- No --> E[Unknown-command error]
D -- Yes --> F[Resolve config, trust, and extension registry]
F --> G[Run command with leased stdin/stdout/stderr]
G --> H{Handler result}
H -- Exit --> I[Retire registry and return exit code]
H -- Delegate --> J[Validate built-in-only argv]
J --> K{Interactive review?}
K -- No --> L[Retire registry and run headless plan]
K -- Yes --> M[Reuse registry in AppBootstrap]
M --> N[Interactive app owns retirement]
Prompt To Fix All With AI### Issue 1
examples/extensions/github-pr/index.ts:404
**Direct environment access**
The new GitHub PR extension defaults both its fetch helper and runtime configuration to `process.env`, bypassing the repository's required validated environment-variable mechanism for `GH_TOKEN` and `GITHUB_TOKEN` and establishing an unsupported configuration-access pattern.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Reviews (1): Last reviewed commit: "feat(extensions): add generic CLI comman..." | Re-trigger Greptile |
| export async function fetchGitHubPullRequestDiff( | ||
| target: ResolvedGitHubPullRequest, | ||
| signal: AbortSignal, | ||
| env: NodeJS.ProcessEnv = process.env, |
There was a problem hiding this comment.
The new GitHub PR extension defaults both its fetch helper and runtime configuration to process.env, bypassing the repository's required validated environment-variable mechanism for GH_TOKEN and GITHUB_TOKEN and establishing an unsupported configuration-access pattern.
Context Used: guidelines.mdc Cursor rule (source)
Prompt To Fix With AI
This is a comment left during a code review.
Path: examples/extensions/github-pr/index.ts
Line: 404
Comment:
**Direct environment access**
The new GitHub PR extension defaults both its fetch helper and runtime configuration to `process.env`, bypassing the repository's required validated environment-variable mechanism for `GH_TOKEN` and `GITHUB_TOKEN` and establishing an unsupported configuration-access pattern.
**Context Used:** guidelines.mdc Cursor rule ([source](https://github.com/modem-dev/modem/blob/main/.cursor/rules/guidelines.mdc))
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
There was a problem hiding this comment.
Thanks — I checked this against Hunk's actual extension contract and repository conventions. The cited rule belongs to modem-dev/modem, not this repository, and Hunk does not expose a validated environment/secrets API to extensions. This example follows Hunk's existing injectable ProcessEnv = process.env boundary, while its tests cover token precedence, malformed header values, and credential-safe errors. I'm keeping the implementation as-is rather than inventing a parallel extension configuration mechanism.
Responded by Pi using gpt-5.6-sol.
This comment was generated by Pi using gpt-5.6-sol
`createLeasedWriter` returned a bare rejected promise when a write arrived after settlement or carried an unsupported chunk type. The normal write path already attaches a handler to its pending promise, but these two revocation paths did not, so a handler that fired a write it never awaited — a `setTimeout` progress line landing after the command returned is the usual shape — produced an unhandled rejection. That crashed the process with a stack trace pointing at Hunk internals and replaced the command's real exit status with 1, defeating the point of validated exits. Reject through a helper that marks the promise handled. Callers that await a revoked write still see the rejection; callers that discard it no longer take down the process. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Hoisting `--extension` and `--no-extensions` ahead of the command token made them parse successfully for every built-in, but only review commands and extension CLI commands actually consume them. `hunk --extension ./x session list` and `hunk --no-extensions markup guide` silently discarded the flag and ran as if it were never passed. Before the hoist these spellings failed loudly with `Unknown command: --extension`, so the seam turned a hard error into a silent no-op. Reject them the way a misplaced `--fast` is already rejected, naming the command that cannot use the flag. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`registerCliCommand` required `summary` and accepted `usage`, then stored both where nothing ever read them: bare `hunk --help` stays static by design, and `hunk ext list` reports installed extensions rather than the commands they register. A required field with no consumer is a shape we would inherit into API v11 without ever having seen it rendered. Give both fields the discovery surface they were written for. When an unknown top-level token reaches extension resolution and no extension claims it, the registry has already been loaded, so the failure can name what the loaded extensions do offer instead of only repeating the bad token. Both strings come from extension code, so `describeExtensionCliCommands` sanitizes them and collapses each to a single line; otherwise a summary carrying newlines and escape sequences could forge host output around a legitimate error. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`ctx.cwd` is the spawned process's own working directory, which macOS reports through the `/var` -> `/private/var` symlink that `tmpdir()` hands back unresolved. The assertion compared it against the unresolved `mkdtempSync` path, so the test failed on macOS while passing wherever `tmpdir()` is already canonical. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
189f399 to
ed1b060
Compare
Summary
cli-toolsandgithub-prexamples, including an installablehunk gh 123workflow that fetches GitHub PR diffs directly and delegates them tohunk patchWhy
Extensions can currently contribute review-time UI and VCS behavior, but the closed top-level CLI switch prevents them from owning preprocessing workflows. This adds one generic command seam rather than introducing service-specific core commands. The GitHub PR example exercises the complete lifecycle without requiring the
ghCLI or a parallel review path.Design notes
--no-extensionsremains a hard stop; explicit paths, config/global providers, and trusted repo providers keep their existing precedencecwd,AbortSignal, streaming stdin, and leased backpressure-aware writersValidation
bun run typecheckbun run lintbun run deps:checkbun run check:docsbun run check:packbun run test:integration: 132 passing, 1 macOS-only skipbun run test:tty-smoke: 9 passingbun run build:binHUNK_TEST_EXECUTABLE=./dist/hunk bun test test/cli/compiled-headless-native-lib.test.ts: 7 passing, including installed-shape literalhunk gh --helphunk gh 123 --repo modem-dev/hunk -- --pager: fetched PR Include untracked files in working-tree diff reviews #123, mounted the TUI, refreshed withr, quit withq, and left no temporary patch directorybun run testreaches 1,626 passing tests and 2 skips, but its wrapper exits nonzero on this machine becauseregisterHostRuntimeModules > does not claim bare specifiers outside registered extension directoriesfails when the temporary outsider can resolve the repository's React. The focused test reproduces unchanged onorigin/mainatd96a860c.Visual evidence
No renderer or interaction model changes. Delegated reviews enter the existing
patchUI; the real tmux validation above exercised the terminal handoff and refresh path.This PR description was generated by Pi using gpt-5.6-sol